{"thread":{"id":"42278","subject":"[Bug?] log -p -W showing the whole file for a patch that adds to the end?","startedAt":"2016-05-10T21:51:03Z","lastAt":"2016-05-31T21:23:19Z","messageCount":27,"participants":["Junio C Hamano","René Scharfe"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"286121","messageId":"xmqqh9e5mvjs.fsf@gitster.mtv.corp.google.com","threadId":"42278","inReplyTo":null,"subject":"[Bug?] log -p -W showing the whole file for a patch that adds to the end?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-10T21:51:03Z","receivedAt":"2016-05-10T21:51:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Just noticed a curiosity:\n\n $ git show -W 3e3ceaa58 quote.c\n\nshows the entire file. The commit in question adds a whole new\nfunction at the end of the file. If I move that addition to just\nbefore the last function the file already had before the change and\namend the commit, \"show -W\" would work as expected, i.e. addition of\nthe function, with three lines before and three lines after context.\n\nThe -W feature was added by 14937c2c (diff: add option to show whole\nfunctions as context, 2011-10-09).\n"},{"id":"286126","messageId":"xmqq4ma5msrd.fsf@gitster.mtv.corp.google.com","threadId":"42278","inReplyTo":"xmqqh9e5mvjs.fsf@gitster.mtv.corp.google.com","subject":"Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-10T22:51:18Z","receivedAt":"2016-05-10T22:51:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Just noticed a curiosity:\n>\n> $ git show -W 3e3ceaa58 quote.c\n>\n> shows the entire file. The commit in question adds a whole new\n> function at the end of the file. If I move that addition to just\n> before the last function the file already had before the change and\n> amend the commit, \"show -W\" would work as expected, i.e. addition of\n> the function, with three lines before and three lines after context.\n>\n> The -W feature was added by 14937c2c (diff: add option to show whole\n> functions as context, 2011-10-09).\n\nA workaround (atEnd) seems to give me a slightly better result, but\nI am not sure if this breaks other corner cases.\n\nThe real problem is that get_func_line() is called with (start,\nlimit) but a hunk that adds to the end does not even enter the loop\nthat goes back (i.e. step = -1) to find the funcline:\n\n\tfor (l = start; l != limit && 0 <= l && l < xe->xdf1.nrec; l += step) {\n\t\tconst char *rec;\n\t\tlong reclen = xdl_get_rec(&xe->xdf1, l, &rec);\n\t\tlong len = ff(rec, reclen, buf, size, xecfg->find_func_priv);\n\nbecause start == xe->xdf1.nrec in that case. We could instead force\nit to enter the loop by decrementing start when it is xe->xdf1.nrec\nand we are going down, but that would show two functions in the\nresulting hunk (one that is the existing function at the end of the\nfile, the other that is added by the patch), which is not better than\nshowing the patch unmodified.\n\n-- >8 --\nSubject: diff -W: do not show the whole file when punting\n\nThe -W option to \"diff\" family of commands allows the pre- and post-\ncontext of hunks extended to cover the previous and the next\n\"function header line\", so that the whole function that is patched\ncan be seen in the hunk.\n\nThe helper function get_func_line() however gets confused when a\nhunk adds a new function at the very end, and returns -1 to signal\nthat it did not find a suitable \"function header line\", i.e. the\nbeginning of previous function. The caller then takes this signal\nand shows from the very beginning of the file. We end up showing\nthe entire file, starting from the very beginning to the end of the\nnewly added lines.\n\nWe can avoid this by using the original hunk boundary when the\nhelper gives up.\n\nSigned-off-by: Junio C Hamano \n---\n xdiff/xemit.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex 993724b..4e58482 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -166,7 +166,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\tif (xecfg->flags & XDL_EMIT_FUNCCONTEXT) {\n \t\t\tlong fs1 = get_func_line(xe, xecfg, NULL, xch->i1, -1);\n \t\t\tif (fs1 < 0)\n-\t\t\t\tfs1 = 0;\n+\t\t\t\tfs1 = xch->i1;\n \t\t\tif (fs1 < s1) {\n \t\t\t\ts2 -= s1 - fs1;\n \t\t\t\ts1 = fs1;\n"},{"id":"286407","messageId":"5734FFBC.5070505@web.de","threadId":"42278","inReplyTo":"xmqq4ma5msrd.fsf@gitster.mtv.corp.google.com","subject":"Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-12T22:12:12Z","receivedAt":"2016-05-12T22:12:12Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.05.2016 um 00:51 schrieb Junio C Hamano:\n> The helper function get_func_line() however gets confused when a\n> hunk adds a new function at the very end, and returns -1 to signal\n> that it did not find a suitable \"function header line\", i.e. the\n> beginning of previous function. The caller then takes this signal\n> and shows from the very beginning of the file. We end up showing\n> the entire file, starting from the very beginning to the end of the\n> newly added lines.\n>\n> We can avoid this by using the original hunk boundary when the\n> helper gives up.\n\nIt's a more conservative fallback in this case, but it regresses e.g. if \nyou have a long list of includes at the top of a C file -- it won't show \nthe full list anymore with -W.\n\nThe problem is that we are trying to access lines in the preimage that \nare only actually in the postimage. Simply switching to useing the \npostimage unconditionally is not enough -- we'd have the same problem \nwith the complementary case of lines at the end being removed.\n\nI wonder if using the postimage as a fallback suffices. Need to look at \nthis more closely.\n\nRené\n"},{"id":"287153","messageId":"5740AC28.6010202@web.de","threadId":"42278","inReplyTo":"xmqq4ma5msrd.fsf@gitster.mtv.corp.google.com","subject":"Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-21T18:42:48Z","receivedAt":"2016-05-21T18:42:48Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.05.2016 um 00:51 schrieb Junio C Hamano:\n> The helper function get_func_line() however gets confused when a\n> hunk adds a new function at the very end, and returns -1 to signal\n> that it did not find a suitable \"function header line\", i.e. the\n> beginning of previous function. The caller then takes this signal\n> and shows from the very beginning of the file. We end up showing\n> the entire file, starting from the very beginning to the end of the\n> newly added lines.\n\nIn this case we need to look at the added lines in the post-image to\nsee if the original context suffices. We currently only look at the\npre-image. And if we have to extend the context simply search from the\nbottom of the pre-image. That's what the second patch does; the first\none is just a small preparation.\n\nThe last three patches introduce special handling of empty lines.\nBefore them the code for -W only distinguished between function lines\nand non-function lines. Not allowing empty lines between functions to\nextend the context with -W is most useful in the case of appended full\nfunctions -- otherwise we'd get the preceding function shown as well as\nit \"belongs\" to the empty line separating them.\n\nAnd if we do that then it's easy and more consistent to stop showing\nempty lines trailing functions with -W (unless they're already included\nin the one requested with -u/-U, of course). Doing the same for grep\nthen is only fair.\n\nConsidering empty lines as uninteresting collides with languages like\nWhitespace. I'm not sure -W was useful for it to begin with, though.\nAny other possible downsides?\n\nNB: Comments are still not handled specially. That means they are\nconsidered to be part of the preceding function. Fixing that is not in\nscope for this series. (And I'm not sure it would be worth the hassle.)\n\n diff: factor out match_func_rec()\n diff: handle appended chunks better with -W\n diff: ignore empty lines before added functions with -W\n diff: don't include common trailing empty lines with -W\n grep: don't extend context to trailing empty lines with -W\n\n grep.c | 28 ++++++++++++++++++++++++--\n xdiff/xemit.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++-------\n 2 files changed, 82 insertions(+), 9 deletions(-)\n"},{"id":"287154","messageId":"5740ACAD.4060901@web.de","threadId":"42278","inReplyTo":"5740AC28.6010202@web.de","subject":"[PATCH 1/5] diff: factor out match_func_rec()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-21T18:45:01Z","receivedAt":"2016-05-21T18:45:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add match_func_rec(), a helper that wraps accessing a record and calling\nthe appropriate function for checking if it contains a function line.\n\nSigned-off-by: Rene Scharfe \n---\nWe'll use it in the next patch. A follow-up patch could inline def_ff()\ninto it.\n\n xdiff/xemit.c | 15 +++++++++++----\n 1 file changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex 993724b..0c87637 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -120,6 +120,16 @@ static long def_ff(const char *rec, long len, char *buf, long sz, void *priv)\n \treturn -1;\n }\n \n+static long match_func_rec(xdfile_t *xdf, xdemitconf_t const *xecfg, long ri,\n+\t\t\t char *buf, long sz)\n+{\n+\tconst char *rec;\n+\tlong len = xdl_get_rec(xdf, ri, &rec);\n+\tif (!xecfg->find_func)\n+\t\treturn def_ff(rec, len, buf, sz, xecfg->find_func_priv);\n+\treturn xecfg->find_func(rec, len, buf, sz, xecfg->find_func_priv);\n+}\n+\n struct func_line {\n \tlong len;\n \tchar buf[80];\n@@ -128,7 +138,6 @@ struct func_line {\n static long get_func_line(xdfenv_t *xe, xdemitconf_t const *xecfg,\n \t\t\t struct func_line *func_line, long start, long limit)\n {\n-\tfind_func_t ff = xecfg->find_func ? xecfg->find_func : def_ff;\n \tlong l, size, step = (start > limit) ? -1 : 1;\n \tchar *buf, dummy[1];\n \n@@ -136,9 +145,7 @@ static long get_func_line(xdfenv_t *xe, xdemitconf_t const *xecfg,\n \tsize = func_line ? sizeof(func_line->buf) : sizeof(dummy);\n \n \tfor (l = start; l != limit && 0 <= l && l < xe->xdf1.nrec; l += step) {\n-\t\tconst char *rec;\n-\t\tlong reclen = xdl_get_rec(&xe->xdf1, l, &rec);\n-\t\tlong len = ff(rec, reclen, buf, size, xecfg->find_func_priv);\n+\t\tlong len = match_func_rec(&xe->xdf1, xecfg, l, buf, size);\n \t\tif (len >= 0) {\n \t\t\tif (func_line)\n \t\t\t\tfunc_line->len = len;\n-- \n2.8.3\n"},{"id":"287155","messageId":"5740ACD7.8010404@web.de","threadId":"42278","inReplyTo":"5740AC28.6010202@web.de","subject":"[PATCH 2/5] diff: handle appended chunks better with -W","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-21T18:45:43Z","receivedAt":"2016-05-21T18:45:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If lines are added at the end of a file, diff -W shows the whole file.\nThat's because get_func_line() only considers the pre-image and gives up\nif it sees a record index beyond its end.\n\nConsider the post-image as well to see if the added lines already make\nup a full function. If it doesn't then search for the previous function\nline by starting from the bottom of the pre-image, thereby avoiding to\nconfuse get_func_line().\n\nReuse the existing label called \"again\", as it's exactly where we need\nto jump to when we're done handling the pre-context, but rename it to\n\"post_context_calculation\" in order to document its new purpose better.\n\nReported-by: Junio C Hamano \nInitial-patch-by: Junio C Hamano \nSigned-off-by: Rene Scharfe \n---\n xdiff/xemit.c | 27 ++++++++++++++++++++++++---\n 1 file changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex 0c87637..969100d 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -171,7 +171,28 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\ts2 = XDL_MAX(xch->i2 - xecfg->ctxlen, 0);\n \n \t\tif (xecfg->flags & XDL_EMIT_FUNCCONTEXT) {\n-\t\t\tlong fs1 = get_func_line(xe, xecfg, NULL, xch->i1, -1);\n+\t\t\tlong fs1, i1 = xch->i1;\n+\n+\t\t\t/* Appended chunk? */\n+\t\t\tif (i1 >= xe->xdf1.nrec) {\n+\t\t\t\tchar dummy[1];\n+\n+\t\t\t\t/*\n+\t\t\t\t * We don't need additional context if\n+\t\t\t\t * a whole function was added.\n+\t\t\t\t */\n+\t\t\t\tif (match_func_rec(&xe->xdf2, xecfg, xch->i2,\n+\t\t\t\t\t\t dummy, sizeof(dummy)) >= 0)\n+\t\t\t\t\tgoto post_context_calculation;\n+\n+\t\t\t\t/*\n+\t\t\t\t * Otherwise get more context from the\n+\t\t\t\t * pre-image.\n+\t\t\t\t */\n+\t\t\t\ti1 = xe->xdf1.nrec - 1;\n+\t\t\t}\n+\n+\t\t\tfs1 = get_func_line(xe, xecfg, NULL, i1, -1);\n \t\t\tif (fs1 < 0)\n \t\t\t\tfs1 = 0;\n \t\t\tif (fs1 < s1) {\n@@ -180,7 +201,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t\t}\n \t\t}\n \n- again:\n+ post_context_calculation:\n \t\tlctx = xecfg->ctxlen;\n \t\tlctx = XDL_MIN(lctx, xe->xdf1.nrec - (xche->i1 + xche->chg1));\n \t\tlctx = XDL_MIN(lctx, xe->xdf2.nrec - (xche->i2 + xche->chg2));\n@@ -209,7 +230,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t\t\tif (l <= e1 ||\n \t\t\t\t get_func_line(xe, xecfg, NULL, l, e1) < 0) {\n \t\t\t\t\txche = xche->next;\n-\t\t\t\t\tgoto again;\n+\t\t\t\t\tgoto post_context_calculation;\n \t\t\t\t}\n \t\t\t}\n \t\t}\n-- \n2.8.3\n"},{"id":"287156","messageId":"5740ACF5.4060107@web.de","threadId":"42278","inReplyTo":"5740AC28.6010202@web.de","subject":"[PATCH 3/5] diff: ignore empty lines before added functions with -W","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-21T18:46:13Z","receivedAt":"2016-05-21T18:46:13Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If a new function and a preceding empty line is appended, diff -W shows\nthe previous function in full in order to provide context for that empty\nline. In most languages empty lines between sections are not \ninteresting in and off themselves and showing a whole extra function for\nthem is not what we want.\n\nSkip empty lines when checking of the appended chunk starts with a\nfunction line, thereby avoiding to extend the context just for them.\n\nSigned-off-by: Rene Scharfe \n---\n xdiff/xemit.c | 22 ++++++++++++++++++++--\n 1 file changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex 969100d..087956a 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -155,6 +155,18 @@ static long get_func_line(xdfenv_t *xe, xdemitconf_t const *xecfg,\n \treturn -1;\n }\n \n+static int is_empty_rec(xdfile_t *xdf, xdemitconf_t const *xecfg, long ri)\n+{\n+\tconst char *rec;\n+\tlong len = xdl_get_rec(xdf, ri, &rec);\n+\n+\twhile (len > 0 && isspace(*rec)) {\n+\t\trec++;\n+\t\tlen--;\n+\t}\n+\treturn !len;\n+}\n+\n int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t xdemitconf_t const *xecfg) {\n \tlong s1, s2, e1, e2, lctx;\n@@ -176,12 +188,18 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t\t/* Appended chunk? */\n \t\t\tif (i1 >= xe->xdf1.nrec) {\n \t\t\t\tchar dummy[1];\n+\t\t\t\tlong i2 = xch->i2;\n \n \t\t\t\t/*\n \t\t\t\t * We don't need additional context if\n-\t\t\t\t * a whole function was added.\n+\t\t\t\t * a whole function was added, possibly\n+\t\t\t\t * starting with empty lines.\n \t\t\t\t */\n-\t\t\t\tif (match_func_rec(&xe->xdf2, xecfg, xch->i2,\n+\t\t\t\twhile (i2 < xe->xdf2.nrec &&\n+\t\t\t\t is_empty_rec(&xe->xdf2, xecfg, i2))\n+\t\t\t\t\ti2++;\n+\t\t\t\tif (i2 < xe->xdf2.nrec &&\n+\t\t\t\t match_func_rec(&xe->xdf2, xecfg, i2,\n \t\t\t\t\t\t dummy, sizeof(dummy)) >= 0)\n \t\t\t\t\tgoto post_context_calculation;\n \n-- \n2.8.3\n"},{"id":"287157","messageId":"5740AD0C.505@web.de","threadId":"42278","inReplyTo":"5740AC28.6010202@web.de","subject":"[PATCH 4/5] diff: don't include common trailing empty lines with -W","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-21T18:46:36Z","receivedAt":"2016-05-21T18:46:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Empty lines between functions are shown by diff -W, as it considers them\nto be part of the function preceding them. They are not interesting in\nmost languages. The previous patch stopped showing them in the special\ncase of a function added at the end of a file.\n\nStop extending context to those empty lines by skipping back over them\nfrom the start of the next function.\n\nSigned-off-by: Rene Scharfe \n---\n xdiff/xemit.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex 087956a..d0c0738 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -231,6 +231,9 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t\tlong fe1 = get_func_line(xe, xecfg, NULL,\n \t\t\t\t\t\t xche->i1 + xche->chg1,\n \t\t\t\t\t\t xe->xdf1.nrec);\n+\t\t\twhile (fe1 > 0 &&\n+\t\t\t is_empty_rec(&xe->xdf1, xecfg, fe1 - 1))\n+\t\t\t\tfe1--;\n \t\t\tif (fe1 < 0)\n \t\t\t\tfe1 = xe->xdf1.nrec;\n \t\t\tif (fe1 > e1) {\n-- \n2.8.3\n"},{"id":"287158","messageId":"5740AD2A.9030905@web.de","threadId":"42278","inReplyTo":"5740AC28.6010202@web.de","subject":"[PATCH 5/5] grep: don't extend context to trailing empty lines with -W","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-21T18:47:06Z","receivedAt":"2016-05-21T18:47:06Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Empty lines between functions are shown by grep -W, as it considers them\nto be part of the function preceding them. They are not interesting in\nmost languages. The previous patches stopped showing them for diff -W.\n\nStop showing empty lines trailing a function with grep -W. Grep scans\nthe lines of a buffer from top to bottom and prints matching lines\nimmediately. Thus we need to peek ahead in order to determine if an\nempty line is part of a function body and worth showing or not.\n\nRemember how far ahead we peeked in order to avoid having to do so\nrepeatedly when handling multiple consecutive empty lines.\n\nSigned-off-by: Rene Scharfe \n---\n grep.c | 28 ++++++++++++++++++++++++++--\n 1 file changed, 26 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex ec6f7ff..1e15b62 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1396,9 +1396,17 @@ static int fill_textconv_grep(struct userdiff_driver *driver,\n \treturn 0;\n }\n \n+static int is_empty_line(const char *bol, const char *eol)\n+{\n+\twhile (bol < eol && isspace(*bol))\n+\t\tbol++;\n+\treturn bol == eol;\n+}\n+\n static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int collect_hits)\n {\n \tchar *bol;\n+\tchar *peek_bol = NULL;\n \tunsigned long left;\n \tunsigned lno = 1;\n \tunsigned last_hit = 0;\n@@ -1543,8 +1551,24 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t\t\t\tshow_function = 1;\n \t\t\tgoto next_line;\n \t\t}\n-\t\tif (show_function && match_funcname(opt, gs, bol, eol))\n-\t\t\tshow_function = 0;\n+\t\tif (show_function && (!peek_bol || peek_bol < bol)) {\n+\t\t\tunsigned long peek_left = left;\n+\t\t\tchar *peek_eol = eol;\n+\n+\t\t\t/*\n+\t\t\t * Trailing empty lines are not interesting.\n+\t\t\t * Peek past them to see if they belong to the\n+\t\t\t * body of the current function.\n+\t\t\t */\n+\t\t\tpeek_bol = bol;\n+\t\t\twhile (is_empty_line(peek_bol, peek_eol)) {\n+\t\t\t\tpeek_bol = peek_eol + 1;\n+\t\t\t\tpeek_eol = end_of_line(peek_bol, &peek_left);\n+\t\t\t}\n+\n+\t\t\tif (match_funcname(opt, gs, peek_bol, peek_eol))\n+\t\t\t\tshow_function = 0;\n+\t\t}\n \t\tif (show_function ||\n \t\t (last_hit && lno <= last_hit + opt->post_context)) {\n \t\t\t/* If the last hit is within the post context,\n-- \n2.8.3\n"},{"id":"287432","messageId":"xmqq7fej5njv.fsf@gitster.mtv.corp.google.com","threadId":"42278","inReplyTo":"5740AC28.6010202@web.de","subject":"Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-24T18:16:36Z","receivedAt":"2016-05-24T18:16:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> diff: factor out match_func_rec()\n> diff: handle appended chunks better with -W\n> diff: ignore empty lines before added functions with -W\n> diff: don't include common trailing empty lines with -W\n> grep: don't extend context to trailing empty lines with -W\n>\n> grep.c | 28 ++++++++++++++++++++++++--\n> xdiff/xemit.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++-------\n> 2 files changed, 82 insertions(+), 9 deletions(-)\n\nThanks. The flow of reasoning in this cover letter was written very\nwell and I think I can agree with all of them.\n\nIt is curious that this much behaviour change does not need any\nchanges in the test scripts. We do not have sufficient coverage,\nperhaps?\n"},{"id":"287560","messageId":"5746B9AE.5050307@web.de","threadId":"42278","inReplyTo":"xmqq7fej5njv.fsf@gitster.mtv.corp.google.com","subject":"Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-26T08:54:06Z","receivedAt":"2016-05-26T08:54:06Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.05.2016 um 20:16 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> diff: factor out match_func_rec()\n>> diff: handle appended chunks better with -W\n>> diff: ignore empty lines before added functions with -W\n>> diff: don't include common trailing empty lines with -W\n>> grep: don't extend context to trailing empty lines with -W\n>>\n>> grep.c | 28 ++++++++++++++++++++++++--\n>> xdiff/xemit.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++-------\n>> 2 files changed, 82 insertions(+), 9 deletions(-)\n\n> It is curious that this much behaviour change does not need any\n> changes in the test scripts. We do not have sufficient coverage,\n> perhaps?\n\nWell, -W is not tested at all. I'll include some in the next round. No \nneed to hurry, it's too late to land in 2.9.0 anyway.\n\nRené\n"},{"id":"287596","messageId":"xmqqr3cowy0x.fsf@gitster.mtv.corp.google.com","threadId":"42278","inReplyTo":"5746B9AE.5050307@web.de","subject":"Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-26T17:05:02Z","receivedAt":"2016-05-26T17:05:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 24.05.2016 um 20:16 schrieb Junio C Hamano:\n>> René Scharfe writes:\n>>\n>>> diff: factor out match_func_rec()\n>>> diff: handle appended chunks better with -W\n>>> diff: ignore empty lines before added functions with -W\n>>> diff: don't include common trailing empty lines with -W\n>>> grep: don't extend context to trailing empty lines with -W\n>>>\n>>> grep.c | 28 ++++++++++++++++++++++++--\n>>> xdiff/xemit.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++-------\n>>> 2 files changed, 82 insertions(+), 9 deletions(-)\n>\n>> It is curious that this much behaviour change does not need any\n>> changes in the test scripts. We do not have sufficient coverage,\n>> perhaps?\n>\n> Well, -W is not tested at all. I'll include some in the next round.\n> No need to hurry, it's too late to land in 2.9.0 anyway.\n\nTrue.\n\nI'd say that these patches are fine as they are, and follow-up patch\nfor adding -W tests (instead of rerolling them) is sufficient,\nthough.\n"},{"id":"287696","messageId":"5748803C.6030007@web.de","threadId":"42278","inReplyTo":"xmqqr3cowy0x.fsf@gitster.mtv.corp.google.com","subject":"Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-27T17:13:32Z","receivedAt":"2016-05-27T17:13:32Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 26.05.2016 um 19:05 schrieb Junio C Hamano:\n> I'd say that these patches are fine as they are, and follow-up patch\n> for adding -W tests (instead of rerolling them) is sufficient,\n> though.\n\nPatch 3 needs two small updates to address the char signedness issue \nfound by Ramsay and to get rid of an unused function parameter, and \npatch 4 needs a small change as a result of the latter as well. \nShouldn't be long before tests are done..\n\nRené\n"},{"id":"287737","messageId":"5749AF59.2070704@web.de","threadId":"42278","inReplyTo":"5740AC28.6010202@web.de","subject":"Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-28T14:46:49Z","receivedAt":"2016-05-28T14:46:49Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.05.2016 um 20:42 schrieb René Scharfe:\n> Am 11.05.2016 um 00:51 schrieb Junio C Hamano:\n>> The helper function get_func_line() however gets confused when a\n>> hunk adds a new function at the very end, and returns -1 to signal\n>> that it did not find a suitable \"function header line\", i.e. the\n>> beginning of previous function. The caller then takes this signal\n>> and shows from the very beginning of the file. We end up showing\n>> the entire file, starting from the very beginning to the end of the\n>> newly added lines.\n> \n> In this case we need to look at the added lines in the post-image to\n> see if the original context suffices. We currently only look at the\n> pre-image. And if we have to extend the context simply search from the\n> bottom of the pre-image. That's what the second patch does; the first\n> one is just a small preparation.\n> \n> The last three patches introduce special handling of empty lines.\n> Before them the code for -W only distinguished between function lines\n> and non-function lines. Not allowing empty lines between functions to\n> extend the context with -W is most useful in the case of appended full\n> functions -- otherwise we'd get the preceding function shown as well as\n> it \"belongs\" to the empty line separating them.\n> \n> And if we do that then it's easy and more consistent to stop showing\n> empty lines trailing functions with -W (unless they're already included\n> in the one requested with -u/-U, of course). Doing the same for grep\n> then is only fair.\n> \n> Considering empty lines as uninteresting collides with languages like\n> Whitespace. I'm not sure -W was useful for it to begin with, though.\n> Any other possible downsides?\n> \n> NB: Comments are still not handled specially. That means they are\n> considered to be part of the preceding function. Fixing that is not in\n> scope for this series. (And I'm not sure it would be worth the hassle.)\n> \n> diff: factor out match_func_rec()\n> diff: handle appended chunks better with -W\n> diff: ignore empty lines before added functions with -W\n> diff: don't include common trailing empty lines with -W\n> grep: don't extend context to trailing empty lines with -W\n> \n> grep.c | 28 ++++++++++++++++++++++++--\n> xdiff/xemit.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++-------\n> 2 files changed, 82 insertions(+), 9 deletions(-)\n\nAnd here's the second round, this time with tests, a fix for a\nsignedness issue found by Ramsay, a small cleanup of the new function\nis_empty_rec() and a new fix for the case of using git diff -W -U0.\nAn interdiff for the files touched by the original series is at the\nbottom.\n\n\n t4051: rewrite, add more tests\n xdiff: factor out match_func_rec()\n xdiff: handle appended chunks better with -W\n xdiff: ignore empty lines before added functions with -W\n xdiff: -W: don't include common trailing empty lines in context\n xdiff: don't trim common tail with -W\n t7810: add test for grep -W and trailing empty context lines\n grep: -W: don't extend context to trailing empty lines\n\n grep.c | 28 ++++-\n t/t4051-diff-function-context.sh | 216 +++++++++++++++++++++++++--------------\n t/t4051/appended1.c | 15 +++\n t/t4051/appended2.c | 35 +++++++\n t/t4051/dummy.c | 7 ++\n t/t4051/hello.c | 21 ++++\n t/t4051/includes.c | 20 ++++\n t/t7810-grep.sh | 19 +++-\n xdiff-interface.c | 10 +-\n xdiff/xemit.c | 62 +++++++++--\n 10 files changed, 341 insertions(+), 92 deletions(-)\n create mode 100644 t/t4051/appended1.c\n create mode 100644 t/t4051/appended2.c\n create mode 100644 t/t4051/dummy.c\n create mode 100644 t/t4051/hello.c\n create mode 100644 t/t4051/includes.c\n\n\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex d0c0738..bfa53d3 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -155,12 +155,12 @@ static long get_func_line(xdfenv_t *xe, xdemitconf_t const *xecfg,\n \treturn -1;\n }\n \n-static int is_empty_rec(xdfile_t *xdf, xdemitconf_t const *xecfg, long ri)\n+static int is_empty_rec(xdfile_t *xdf, long ri)\n {\n \tconst char *rec;\n \tlong len = xdl_get_rec(xdf, ri, &rec);\n \n-\twhile (len > 0 && isspace(*rec)) {\n+\twhile (len > 0 && XDL_ISSPACE(*rec)) {\n \t\trec++;\n \t\tlen--;\n \t}\n@@ -196,7 +196,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t\t\t * starting with empty lines.\n \t\t\t\t */\n \t\t\t\twhile (i2 < xe->xdf2.nrec &&\n-\t\t\t\t is_empty_rec(&xe->xdf2, xecfg, i2))\n+\t\t\t\t is_empty_rec(&xe->xdf2, i2))\n \t\t\t\t\ti2++;\n \t\t\t\tif (i2 < xe->xdf2.nrec &&\n \t\t\t\t match_func_rec(&xe->xdf2, xecfg, i2,\n@@ -231,8 +231,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t\tlong fe1 = get_func_line(xe, xecfg, NULL,\n \t\t\t\t\t\t xche->i1 + xche->chg1,\n \t\t\t\t\t\t xe->xdf1.nrec);\n-\t\t\twhile (fe1 > 0 &&\n-\t\t\t is_empty_rec(&xe->xdf1, xecfg, fe1 - 1))\n+\t\t\twhile (fe1 > 0 && is_empty_rec(&xe->xdf1, fe1 - 1))\n \t\t\t\tfe1--;\n \t\t\tif (fe1 < 0)\n \t\t\t\tfe1 = xe->xdf1.nrec;\n"},{"id":"287738","messageId":"5749B1EA.10707@web.de","threadId":"42278","inReplyTo":"5749AF59.2070704@web.de","subject":"[PATCH v2 1/8] t4051: rewrite, add more tests","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-28T14:57:46Z","receivedAt":"2016-05-28T14:57:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Remove the tests that checked diff -W output against a fixed expected\nresult and replace them with more focused checks of desired properties\nof the created diffs. That way we get more detailed and meaningful\ndiagnostics.\n\nStore test file contents in files in a subdirectory in order to avoid\ncluttering the test script with them.\n\nUse tagged commits to store the changes to test diff -W against instead\nof using changes to the worktree. Use the worktree instead to try and\napply the generated patch in order to validate it.\n\nDocument unwanted features: trailing empty lines, too much context for\nappended functions, insufficient context at the end with -U0.\n\nSigned-off-by: Rene Scharfe \n---\n t/t4051-diff-function-context.sh | 216 +++++++++++++++++++++++++--------------\n t/t4051/appended1.c | 15 +++\n t/t4051/appended2.c | 35 +++++++\n t/t4051/dummy.c | 7 ++\n t/t4051/hello.c | 21 ++++\n t/t4051/includes.c | 20 ++++\n 6 files changed, 240 insertions(+), 74 deletions(-)\n create mode 100644 t/t4051/appended1.c\n create mode 100644 t/t4051/appended2.c\n create mode 100644 t/t4051/dummy.c\n create mode 100644 t/t4051/hello.c\n create mode 100644 t/t4051/includes.c\n\ndiff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh\nindex 001d678..a3adba4 100755\n--- a/t/t4051-diff-function-context.sh\n+++ b/t/t4051-diff-function-context.sh\n@@ -3,90 +3,158 @@\n test_description='diff function context'\n \n . ./test-lib.sh\n-. \"$TEST_DIRECTORY\"/diff-lib.sh\n \n+dir=\"$TEST_DIRECTORY/t4051\"\n \n-cat <<\\EOF >hello.c\n-#include \n-\n-static int a(void)\n-{\n-\t/*\n-\t * Dummy.\n-\t */\n+commit_and_tag () {\n+\tmessage=$1 &&\n+\tshift &&\n+\tgit add $@ &&\n+\ttest_tick &&\n+\tgit commit -m $message &&\n+\tgit tag $message\n }\n \n-static int hello_world(void)\n-{\n-\t/* Classic. */\n-\tprintf(\"Hello world.\\n\");\n-\n-\t/* Success! */\n-\treturn 0;\n+first_context_line () {\n+\tawk '\n+\t\tfound {print; exit}\n+\t\t/^@@/ {found = 1}\n+\t'\n }\n-static int b(void)\n-{\n-\t/*\n-\t * Dummy, too.\n-\t */\n+\n+last_context_line () {\n+\tsed -n '$ p'\n }\n \n-int main(int argc, char **argv)\n-{\n-\ta();\n-\tb();\n-\treturn hello_world();\n+check_diff () {\n+\tname=$1\n+\tdesc=$2\n+\toptions=\"-W $3\"\n+\n+\ttest_expect_success \"$desc\" '\n+\t\tgit diff $options \"$name^\" \"$name\" >\"$name.diff\"\n+\t'\n+\n+\ttest_expect_success ' diff applies' '\n+\t\ttest_when_finished \"git reset --hard\" &&\n+\t\tgit checkout --detach \"$name^\" &&\n+\t\tgit apply \"$name.diff\" &&\n+\t\tgit diff --exit-code \"$name\"\n+\t'\n }\n-EOF\n \n test_expect_success 'setup' '\n-\tgit add hello.c &&\n-\ttest_tick &&\n-\tgit commit -m initial &&\n-\n-\tgrep -v Classic hello.c.new &&\n-\tmv hello.c.new hello.c\n-'\n-\n-cat <<\\EOF >expected\n-diff --git a/hello.c b/hello.c\n---- a/hello.c\n-+++ b/hello.c\n-@@ -10,8 +10,7 @@ static int a(void)\n- static int hello_world(void)\n- {\n--\t/* Classic. */\n- \tprintf(\"Hello world.\\n\");\n- \n- \t/* Success! */\n- \treturn 0;\n- }\n-EOF\n-\n-test_expect_success 'diff -U0 -W' '\n-\tgit diff -U0 -W >actual &&\n-\tcompare_diff_patch actual expected\n-'\n-\n-cat <<\\EOF >expected\n-diff --git a/hello.c b/hello.c\n---- a/hello.c\n-+++ b/hello.c\n-@@ -9,9 +9,8 @@ static int a(void)\n- \n- static int hello_world(void)\n- {\n--\t/* Classic. */\n- \tprintf(\"Hello world.\\n\");\n- \n- \t/* Success! */\n- \treturn 0;\n- }\n-EOF\n-\n-test_expect_success 'diff -W' '\n-\tgit diff -W >actual &&\n-\tcompare_diff_patch actual expected\n+\tcat \"$dir/includes.c\" \"$dir/dummy.c\" \"$dir/dummy.c\" \"$dir/hello.c\" \\\n+\t\t\"$dir/dummy.c\" \"$dir/dummy.c\" >file.c &&\n+\tcommit_and_tag initial file.c &&\n+\n+\tgrep -v \"delete me from hello\" file.c.new &&\n+\tmv file.c.new file.c &&\n+\tcommit_and_tag changed_hello file.c &&\n+\n+\tgrep -v \"delete me from includes\" file.c.new &&\n+\tmv file.c.new file.c &&\n+\tcommit_and_tag changed_includes file.c &&\n+\n+\tcat \"$dir/appended1.c\" >>file.c &&\n+\tcommit_and_tag appended file.c &&\n+\n+\tcat \"$dir/appended2.c\" >>file.c &&\n+\tcommit_and_tag extended file.c &&\n+\n+\tgrep -v \"Begin of second part\" file.c.new &&\n+\tmv file.c.new file.c &&\n+\tcommit_and_tag long_common_tail file.c\n+'\n+\n+check_diff changed_hello 'changed function'\n+\n+test_expect_success ' context includes begin' '\n+\tgrep \"^ .*Begin of hello\" changed_hello.diff\n+'\n+\n+test_expect_success ' context includes end' '\n+\tgrep \"^ .*End of hello\" changed_hello.diff\n+'\n+\n+test_expect_success ' context does not include other functions' '\n+\ttest $(grep -c \"^[ +-].*Begin\" changed_hello.diff) -le 1\n+'\n+\n+test_expect_success ' context does not include preceding empty lines' '\n+\ttest \"$(first_context_line = 0;\n+\t i--) {\n+\t\tprintf(\"%d bottles of beer on the wall\\n\", i);\n+\t}\n+\n+\tprintf(\"End of first part\\n\");\ndiff --git a/t/t4051/appended2.c b/t/t4051/appended2.c\nnew file mode 100644\nindex 0000000..e651f71\n--- /dev/null\n+++ b/t/t4051/appended2.c\n@@ -0,0 +1,35 @@\n+\tprintf(\"Begin of second part\\n\");\n+\n+\t/*\n+\t * Lorem ipsum dolor sit amet, consectetuer sadipscing elitr,\n+\t * sed diam nonumy eirmod tempor invidunt ut labore et dolore\n+\t * magna aliquyam erat, sed diam voluptua. At vero eos et\n+\t * accusam et justo duo dolores et ea rebum. Stet clita kasd\n+\t * gubergren, no sea takimata sanctus est Lorem ipsum dolor\n+\t * sit amet.\n+\t *\n+\t * Lorem ipsum dolor sit amet, consectetuer sadipscing elitr,\n+\t * sed diam nonumy eirmod tempor invidunt ut labore et dolore\n+\t * magna aliquyam erat, sed diam voluptua. At vero eos et\n+\t * accusam et justo duo dolores et ea rebum. Stet clita kasd\n+\t * gubergren, no sea takimata sanctus est Lorem ipsum dolor\n+\t * sit amet.\n+\t *\n+\t * Lorem ipsum dolor sit amet, consectetuer sadipscing elitr,\n+\t * sed diam nonumy eirmod tempor invidunt ut labore et dolore\n+\t * magna aliquyam erat, sed diam voluptua. At vero eos et\n+\t * accusam et justo duo dolores et ea rebum. Stet clita kasd\n+\t * gubergren, no sea takimata sanctus est Lorem ipsum dolor\n+\t * sit amet.\n+\t *\n+\t * Lorem ipsum dolor sit amet, consectetuer sadipscing elitr,\n+\t * sed diam nonumy eirmod tempor invidunt ut labore et dolore\n+\t * magna aliquyam erat, sed diam voluptua. At vero eos et\n+\t * accusam et justo duo dolores et ea rebum. Stet clita kasd\n+\t * gubergren, no sea takimata sanctus est Lorem ipsum dolor\n+\t * sit amet.\n+\t *\n+\t */\n+\n+\treturn 0;\n+}\t// End of second part\ndiff --git a/t/t4051/dummy.c b/t/t4051/dummy.c\nnew file mode 100644\nindex 0000000..a43016e\n--- /dev/null\n+++ b/t/t4051/dummy.c\n@@ -0,0 +1,7 @@\n+\n+static int dummy(void)\t// Begin of dummy\n+{\n+\tint rc = 0;\n+\n+\treturn rc;\n+}\t// End of dummy\ndiff --git a/t/t4051/hello.c b/t/t4051/hello.c\nnew file mode 100644\nindex 0000000..63b1a1e\n--- /dev/null\n+++ b/t/t4051/hello.c\n@@ -0,0 +1,21 @@\n+\n+static void hello(void)\t// Begin of hello\n+{\n+\t/*\n+\t * Classic.\n+\t */\n+\tputchar('H');\n+\tputchar('e');\n+\tputchar('l');\n+\tputchar('l');\n+\tputchar('o');\n+\tputchar(' ');\n+\t/* delete me from hello */\n+\tputchar('w');\n+\tputchar('o');\n+\tputchar('r');\n+\tputchar('l');\n+\tputchar('d');\n+\tputchar('.');\n+\tputchar('\\n');\n+}\t// End of hello\ndiff --git a/t/t4051/includes.c b/t/t4051/includes.c\nnew file mode 100644\nindex 0000000..efc68f8\n--- /dev/null\n+++ b/t/t4051/includes.c\n@@ -0,0 +1,20 @@\n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+/* delete me from includes */\n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n-- \n2.8.3\n"},{"id":"287739","messageId":"5749B227.2020307@web.de","threadId":"42278","inReplyTo":"5749AF59.2070704@web.de","subject":"[PATCH v2 2/8] xdiff: factor out match_func_rec()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-28T14:58:47Z","receivedAt":"2016-05-28T14:58:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add match_func_rec(), a helper that wraps accessing a record and calling\nthe appropriate function for checking if it contains a function line.\n\nSigned-off-by: Rene Scharfe \n---\n xdiff/xemit.c | 15 +++++++++++----\n 1 file changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex 993724b..0c87637 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -120,6 +120,16 @@ static long def_ff(const char *rec, long len, char *buf, long sz, void *priv)\n \treturn -1;\n }\n \n+static long match_func_rec(xdfile_t *xdf, xdemitconf_t const *xecfg, long ri,\n+\t\t\t char *buf, long sz)\n+{\n+\tconst char *rec;\n+\tlong len = xdl_get_rec(xdf, ri, &rec);\n+\tif (!xecfg->find_func)\n+\t\treturn def_ff(rec, len, buf, sz, xecfg->find_func_priv);\n+\treturn xecfg->find_func(rec, len, buf, sz, xecfg->find_func_priv);\n+}\n+\n struct func_line {\n \tlong len;\n \tchar buf[80];\n@@ -128,7 +138,6 @@ struct func_line {\n static long get_func_line(xdfenv_t *xe, xdemitconf_t const *xecfg,\n \t\t\t struct func_line *func_line, long start, long limit)\n {\n-\tfind_func_t ff = xecfg->find_func ? xecfg->find_func : def_ff;\n \tlong l, size, step = (start > limit) ? -1 : 1;\n \tchar *buf, dummy[1];\n \n@@ -136,9 +145,7 @@ static long get_func_line(xdfenv_t *xe, xdemitconf_t const *xecfg,\n \tsize = func_line ? sizeof(func_line->buf) : sizeof(dummy);\n \n \tfor (l = start; l != limit && 0 <= l && l < xe->xdf1.nrec; l += step) {\n-\t\tconst char *rec;\n-\t\tlong reclen = xdl_get_rec(&xe->xdf1, l, &rec);\n-\t\tlong len = ff(rec, reclen, buf, size, xecfg->find_func_priv);\n+\t\tlong len = match_func_rec(&xe->xdf1, xecfg, l, buf, size);\n \t\tif (len >= 0) {\n \t\t\tif (func_line)\n \t\t\t\tfunc_line->len = len;\n-- \n2.8.3\n"},{"id":"287740","messageId":"5749B28C.5010006@web.de","threadId":"42278","inReplyTo":"5749AF59.2070704@web.de","subject":"[PATCH v2 3/8] xdiff: handle appended chunks better with -W","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-28T15:00:28Z","receivedAt":"2016-05-28T15:00:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If lines are added at the end of a file, diff -W shows the whole file.\nThat's because get_func_line() only considers the pre-image and gives up\nif it sees a record index beyond its end.\n\nConsider the post-image as well to see if the added lines already make\nup a full function. If it doesn't then search for the previous function\nline by starting from the bottom of the pre-image, thereby avoiding to\nconfuse get_func_line().\n\nReuse the existing label called \"again\", as it's exactly where we need\nto jump to when we're done handling the pre-context, but rename it to\n\"post_context_calculation\" in order to document its new purpose better.\n\nReported-by: Junio C Hamano \nInitial-patch-by: Junio C Hamano \nSigned-off-by: Rene Scharfe \n---\n t/t4051-diff-function-context.sh | 2 +-\n xdiff/xemit.c | 27 ++++++++++++++++++++++++---\n 2 files changed, 25 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh\nindex a3adba4..d78461d 100755\n--- a/t/t4051-diff-function-context.sh\n+++ b/t/t4051-diff-function-context.sh\n@@ -131,7 +131,7 @@ test_expect_success ' context includes end' '\n \tgrep \"^[+].*End of second part\" extended.diff\n '\n \n-test_expect_failure ' context does not include other functions' '\n+test_expect_success ' context does not include other functions' '\n \ttest $(grep -c \"^[ +-].*Begin\" extended.diff) -le 2\n '\n \ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex 0c87637..969100d 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -171,7 +171,28 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\ts2 = XDL_MAX(xch->i2 - xecfg->ctxlen, 0);\n \n \t\tif (xecfg->flags & XDL_EMIT_FUNCCONTEXT) {\n-\t\t\tlong fs1 = get_func_line(xe, xecfg, NULL, xch->i1, -1);\n+\t\t\tlong fs1, i1 = xch->i1;\n+\n+\t\t\t/* Appended chunk? */\n+\t\t\tif (i1 >= xe->xdf1.nrec) {\n+\t\t\t\tchar dummy[1];\n+\n+\t\t\t\t/*\n+\t\t\t\t * We don't need additional context if\n+\t\t\t\t * a whole function was added.\n+\t\t\t\t */\n+\t\t\t\tif (match_func_rec(&xe->xdf2, xecfg, xch->i2,\n+\t\t\t\t\t\t dummy, sizeof(dummy)) >= 0)\n+\t\t\t\t\tgoto post_context_calculation;\n+\n+\t\t\t\t/*\n+\t\t\t\t * Otherwise get more context from the\n+\t\t\t\t * pre-image.\n+\t\t\t\t */\n+\t\t\t\ti1 = xe->xdf1.nrec - 1;\n+\t\t\t}\n+\n+\t\t\tfs1 = get_func_line(xe, xecfg, NULL, i1, -1);\n \t\t\tif (fs1 < 0)\n \t\t\t\tfs1 = 0;\n \t\t\tif (fs1 < s1) {\n@@ -180,7 +201,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t\t}\n \t\t}\n \n- again:\n+ post_context_calculation:\n \t\tlctx = xecfg->ctxlen;\n \t\tlctx = XDL_MIN(lctx, xe->xdf1.nrec - (xche->i1 + xche->chg1));\n \t\tlctx = XDL_MIN(lctx, xe->xdf2.nrec - (xche->i2 + xche->chg2));\n@@ -209,7 +230,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t\t\tif (l <= e1 ||\n \t\t\t\t get_func_line(xe, xecfg, NULL, l, e1) < 0) {\n \t\t\t\t\txche = xche->next;\n-\t\t\t\t\tgoto again;\n+\t\t\t\t\tgoto post_context_calculation;\n \t\t\t\t}\n \t\t\t}\n \t\t}\n-- \n2.8.3\n"},{"id":"287742","messageId":"5749B300.3000804@web.de","threadId":"42278","inReplyTo":"5749AF59.2070704@web.de","subject":"[PATCH v2 4/8] xdiff: ignore empty lines before added functions with -W","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-28T15:02:24Z","receivedAt":"2016-05-28T15:02:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If a new function and a preceding empty line is appended, diff -W shows\nthe previous function in full in order to provide context for that empty\nline. In most languages empty lines between sections are not\ninteresting in and off themselves and showing a whole extra function for\nthem is not what we want.\n\nSkip empty lines when checking of the appended chunk starts with a\nfunction line, thereby avoiding to extend the context just for them.\n\nHelped-by: Ramsay Jones \nSigned-off-by: Rene Scharfe \n---\n t/t4051-diff-function-context.sh | 2 +-\n xdiff/xemit.c | 22 ++++++++++++++++++++--\n 2 files changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh\nindex d78461d..b191c655 100755\n--- a/t/t4051-diff-function-context.sh\n+++ b/t/t4051-diff-function-context.sh\n@@ -117,7 +117,7 @@ test_expect_success ' context includes end' '\n \tgrep \"^[+].*End of first part\" appended.diff\n '\n \n-test_expect_failure ' context does not include other functions' '\n+test_expect_success ' context does not include other functions' '\n \ttest $(grep -c \"^[ +-].*Begin\" appended.diff) -le 1\n '\n \ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex 969100d..29cec12 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -155,6 +155,18 @@ static long get_func_line(xdfenv_t *xe, xdemitconf_t const *xecfg,\n \treturn -1;\n }\n \n+static int is_empty_rec(xdfile_t *xdf, long ri)\n+{\n+\tconst char *rec;\n+\tlong len = xdl_get_rec(xdf, ri, &rec);\n+\n+\twhile (len > 0 && XDL_ISSPACE(*rec)) {\n+\t\trec++;\n+\t\tlen--;\n+\t}\n+\treturn !len;\n+}\n+\n int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t xdemitconf_t const *xecfg) {\n \tlong s1, s2, e1, e2, lctx;\n@@ -176,12 +188,18 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t\t/* Appended chunk? */\n \t\t\tif (i1 >= xe->xdf1.nrec) {\n \t\t\t\tchar dummy[1];\n+\t\t\t\tlong i2 = xch->i2;\n \n \t\t\t\t/*\n \t\t\t\t * We don't need additional context if\n-\t\t\t\t * a whole function was added.\n+\t\t\t\t * a whole function was added, possibly\n+\t\t\t\t * starting with empty lines.\n \t\t\t\t */\n-\t\t\t\tif (match_func_rec(&xe->xdf2, xecfg, xch->i2,\n+\t\t\t\twhile (i2 < xe->xdf2.nrec &&\n+\t\t\t\t is_empty_rec(&xe->xdf2, i2))\n+\t\t\t\t\ti2++;\n+\t\t\t\tif (i2 < xe->xdf2.nrec &&\n+\t\t\t\t match_func_rec(&xe->xdf2, xecfg, i2,\n \t\t\t\t\t\t dummy, sizeof(dummy)) >= 0)\n \t\t\t\t\tgoto post_context_calculation;\n \n-- \n2.8.3\n"},{"id":"287743","messageId":"5749B334.7030404@web.de","threadId":"42278","inReplyTo":"5749AF59.2070704@web.de","subject":"[PATCH v2 5/8] xdiff: -W: don't include common trailing empty lines in context","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-28T15:03:16Z","receivedAt":"2016-05-28T15:03:16Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Empty lines between functions are shown by diff -W, as it considers them\nto be part of the function preceding them. They are not interesting in\nmost languages. The previous patch stopped showing them in the special\ncase of a function added at the end of a file.\n\nStop extending context to those empty lines by skipping back over them\nfrom the start of the next function.\n\nSigned-off-by: Rene Scharfe \n---\n t/t4051-diff-function-context.sh | 4 ++--\n xdiff/xemit.c | 2 ++\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh\nindex b191c655..9fe590f 100755\n--- a/t/t4051-diff-function-context.sh\n+++ b/t/t4051-diff-function-context.sh\n@@ -85,7 +85,7 @@ test_expect_success ' context does not include preceding empty lines' '\n \ttest \"$(first_context_line i1 + xche->chg1,\n \t\t\t\t\t\t xe->xdf1.nrec);\n+\t\t\twhile (fe1 > 0 && is_empty_rec(&xe->xdf1, fe1 - 1))\n+\t\t\t\tfe1--;\n \t\t\tif (fe1 < 0)\n \t\t\t\tfe1 = xe->xdf1.nrec;\n \t\t\tif (fe1 > e1) {\n-- \n2.8.3\n"},{"id":"287745","messageId":"5749B37F.4080301@web.de","threadId":"42278","inReplyTo":"5749AF59.2070704@web.de","subject":"[PATCH v2 6/8] xdiff: don't trim common tail with -W","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-28T15:04:31Z","receivedAt":"2016-05-28T15:04:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The function trim_common_tail() exits early if context lines are\nrequested. If -U0 and -W are specified together then it can still trim\ncontext lines that might belong to a changed function. As a result\nthat function is shown incompletely.\n\nFix that by calling trim_common_tail() only if no function context or\nfixed context is requested. The parameter ctx is no longer needed now;\nremove it.\n\nWhile at it fix an outdated comment as well.\n\nSigned-off-by: Rene Scharfe \n---\n t/t4051-diff-function-context.sh | 2 +-\n xdiff-interface.c | 10 ++++------\n 2 files changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh\nindex 9fe590f..88a3308 100755\n--- a/t/t4051-diff-function-context.sh\n+++ b/t/t4051-diff-function-context.sh\n@@ -145,7 +145,7 @@ test_expect_success ' context includes begin' '\n \tgrep \"^ .*Begin of first part\" long_common_tail.diff\n '\n \n-test_expect_failure ' context includes end' '\n+test_expect_success ' context includes end' '\n \tgrep \"^ .*End of second part\" long_common_tail.diff\n '\n \ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 54236f2..f34ea76 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -100,9 +100,9 @@ static int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)\n \n /*\n * Trim down common substring at the end of the buffers,\n- * but leave at least ctx lines at the end.\n+ * but end on a complete line.\n */\n-static void trim_common_tail(mmfile_t *a, mmfile_t *b, long ctx)\n+static void trim_common_tail(mmfile_t *a, mmfile_t *b)\n {\n \tconst int blk = 1024;\n \tlong trimmed = 0, recovered = 0;\n@@ -110,9 +110,6 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b, long ctx)\n \tchar *bp = b->ptr + b->size;\n \tlong smaller = (a->size < b->size) ? a->size : b->size;\n \n-\tif (ctx)\n-\t\treturn;\n-\n \twhile (blk + trimmed <= smaller && !memcmp(ap - blk, bp - blk, blk)) {\n \t\ttrimmed += blk;\n \t\tap -= blk;\n@@ -134,7 +131,8 @@ int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t co\n \tif (mf1->size > MAX_XDIFF_SIZE || mf2->size > MAX_XDIFF_SIZE)\n \t\treturn -1;\n \n-\ttrim_common_tail(&a, &b, xecfg->ctxlen);\n+\tif (!xecfg->ctxlen && !(xecfg->flags & XDL_EMIT_FUNCCONTEXT))\n+\t\ttrim_common_tail(&a, &b);\n \n \treturn xdl_diff(&a, &b, xpp, xecfg, xecb);\n }\n-- \n2.8.3\n"},{"id":"287746","messageId":"5749B3C5.4060809@web.de","threadId":"42278","inReplyTo":"5749AF59.2070704@web.de","subject":"[PATCH v2 7/8] t7810: add test for grep -W and trailing empty context lines","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-28T15:05:41Z","receivedAt":"2016-05-28T15:05:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a test demonstrating that git grep -W prints empty lines following\nthe function context we're actually interested in. The modified test\nfile makes it necessary to adjust three unrelated test cases.\n\nSigned-off-by: Rene Scharfe \n---\n t/t7810-grep.sh | 19 ++++++++++++++++---\n 1 file changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 1e72971..26912dc 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -9,7 +9,9 @@ test_description='git grep various.\n . ./test-lib.sh\n \n cat >hello.c <\n #include \n+\n int main(int argc, const char **argv)\n {\n \tprintf(\"Hello world.\\n\");\n@@ -715,6 +717,7 @@ test_expect_success 'grep -p' '\n \n cat >expected <\n+hello.c-\n hello.c=int main(int argc, const char **argv)\n hello.c-{\n hello.c-\tprintf(\"Hello world.\\n\");\n@@ -741,6 +744,16 @@ test_expect_success 'grep -W' '\n '\n \n cat >expected <\n+hello.c:#include \n+EOF\n+\n+test_expect_failure 'grep -W shows no trailing empty lines' '\n+\tgit grep -W stdio >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected <expected <hello.c\n-2:int main(int argc, const char **argv)\n-6:\t/* char ?? */\n+4:int main(int argc, const char **argv)\n+8:\t/* char ?? */\n \n hello_world\n 3:Hello_world\n@@ -1340,7 +1353,7 @@ test_expect_success 'grep --color -e A --and --not -e B with context' '\n '\n \n cat >expected <\n+hello.c-\n hello.c=int main(int argc, const char **argv)\n hello.c-{\n hello.c:\tprintf(\"Hello world.\\n\");\n-- \n2.8.3\n"},{"id":"287747","messageId":"5749B3EB.7090308@web.de","threadId":"42278","inReplyTo":"5749AF59.2070704@web.de","subject":"[PATCH v2 8/8] grep: -W: don't extend context to trailing empty lines","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-28T15:06:19Z","receivedAt":"2016-05-28T15:06:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Empty lines between functions are shown by grep -W, as it considers them\nto be part of the function preceding them. They are not interesting in\nmost languages. The previous patches stopped showing them for diff -W.\n\nStop showing empty lines trailing a function with grep -W. Grep scans\nthe lines of a buffer from top to bottom and prints matching lines\nimmediately. Thus we need to peek ahead in order to determine if an\nempty line is part of a function body and worth showing or not.\n\nRemember how far ahead we peeked in order to avoid having to do so\nrepeatedly when handling multiple consecutive empty lines.\n\nSigned-off-by: Rene Scharfe \n---\n grep.c | 28 ++++++++++++++++++++++++++--\n t/t7810-grep.sh | 2 +-\n 2 files changed, 27 insertions(+), 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex ec6f7ff..1e15b62 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1396,9 +1396,17 @@ static int fill_textconv_grep(struct userdiff_driver *driver,\n \treturn 0;\n }\n \n+static int is_empty_line(const char *bol, const char *eol)\n+{\n+\twhile (bol < eol && isspace(*bol))\n+\t\tbol++;\n+\treturn bol == eol;\n+}\n+\n static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int collect_hits)\n {\n \tchar *bol;\n+\tchar *peek_bol = NULL;\n \tunsigned long left;\n \tunsigned lno = 1;\n \tunsigned last_hit = 0;\n@@ -1543,8 +1551,24 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t\t\t\tshow_function = 1;\n \t\t\tgoto next_line;\n \t\t}\n-\t\tif (show_function && match_funcname(opt, gs, bol, eol))\n-\t\t\tshow_function = 0;\n+\t\tif (show_function && (!peek_bol || peek_bol < bol)) {\n+\t\t\tunsigned long peek_left = left;\n+\t\t\tchar *peek_eol = eol;\n+\n+\t\t\t/*\n+\t\t\t * Trailing empty lines are not interesting.\n+\t\t\t * Peek past them to see if they belong to the\n+\t\t\t * body of the current function.\n+\t\t\t */\n+\t\t\tpeek_bol = bol;\n+\t\t\twhile (is_empty_line(peek_bol, peek_eol)) {\n+\t\t\t\tpeek_bol = peek_eol + 1;\n+\t\t\t\tpeek_eol = end_of_line(peek_bol, &peek_left);\n+\t\t\t}\n+\n+\t\t\tif (match_funcname(opt, gs, peek_bol, peek_eol))\n+\t\t\t\tshow_function = 0;\n+\t\t}\n \t\tif (show_function ||\n \t\t (last_hit && lno <= last_hit + opt->post_context)) {\n \t\t\t/* If the last hit is within the post context,\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 26912dc..960425a 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -748,7 +748,7 @@ hello.c-#include \n hello.c:#include \n EOF\n \n-test_expect_failure 'grep -W shows no trailing empty lines' '\n+test_expect_success 'grep -W shows no trailing empty lines' '\n \tgit grep -W stdio >actual &&\n \ttest_cmp expected actual\n '\n-- \n2.8.3\n"},{"id":"287767","messageId":"xmqqinxwpgfn.fsf@gitster.mtv.corp.google.com","threadId":"42278","inReplyTo":"5749B1EA.10707@web.de","subject":"Re: [PATCH v2 1/8] t4051: rewrite, add more tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-29T23:55:56Z","receivedAt":"2016-05-29T23:55:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> +commit_and_tag () {\n> +\tmessage=$1 &&\n> +\tshift &&\n> +\tgit add $@ &&\n\nLack of dq around $@ makes me wonder if there is something funny\ngoing on (looking at the callers, there isn't, so we'd better quote\nit to avoid wasting time, I think).\n\n> +\ttest_tick &&\n> +\tgit commit -m $message &&\n> +\tgit tag $message\n> }\n\nThe use of $message as the sole argument to \"git tag\" makes the\nreaders guess that it must be a single token without any funny\ncharacter, so the readers would probably do not waste too much time\nwondreing if the lack of dq around $message in the last two is\nproblematic.\n\n> +last_context_line () {\n> +\tsed -n '$ p'\n> }\n\nI have a vague recollection that some implementations of sed are\nunhappy to see that space between the address and the operation; I'd\nfeel safer without it.\n\n> +check_diff () {\n> +\tname=$1\n> +\tdesc=$2\n> +\toptions=\"-W $3\"\n> +\n> +\ttest_expect_success \"$desc\" '\n> +\t\tgit diff $options \"$name^\" \"$name\" >\"$name.diff\"\n> +\t'\n> +\n> +\ttest_expect_success ' diff applies' '\n> +\t\ttest_when_finished \"git reset --hard\" &&\n> +\t\tgit checkout --detach \"$name^\" &&\n\nWith the presence of ^ there, --detach is unnecessary; it would not\nhurt, though.\n\n> +\t\tgit apply \"$name.diff\" &&\n> +\t\tgit diff --exit-code \"$name\"\n\nEven though we may know that $name.diff\" will never have a creation\nof new paths, I'd feel safer if \"apply\" is run with \"--index\".\n\n\nThanks.\n"},{"id":"287852","messageId":"574CA8C7.8070807@web.de","threadId":"42278","inReplyTo":"xmqqinxwpgfn.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/8] t4051: rewrite, add more tests","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-30T20:55:35Z","receivedAt":"2016-05-30T20:55:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.05.2016 um 01:55 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> +commit_and_tag () {\n>> +\tmessage=$1 &&\n>> +\tshift &&\n>> +\tgit add $@ &&\n>\n> Lack of dq around $@ makes me wonder if there is something funny\n> going on (looking at the callers, there isn't, so we'd better quote\n> it to avoid wasting time, I think).\n\nOK.\n\n>> +\ttest_tick &&\n>> +\tgit commit -m $message &&\n>> +\tgit tag $message\n>> }\n>\n> The use of $message as the sole argument to \"git tag\" makes the\n> readers guess that it must be a single token without any funny\n> character, so the readers would probably do not waste too much time\n> wondreing if the lack of dq around $message in the last two is\n> problematic.\n\nWell, let's call it $tag; $message is a bit misleading here. The saved \nletters can be invested in quotes. ;)\n\n>> +last_context_line () {\n>> +\tsed -n '$ p'\n>> }\n>\n> I have a vague recollection that some implementations of sed are\n> unhappy to see that space between the address and the operation; I'd\n> feel safer without it.\n\nIndeed most sed calls in t/ have no space there (found counter-examples \nonly in annotate-tests.sh, t4201-shortlog.sh, t9824-git-p4-git-lfs.sh).\n\n>> +check_diff () {\n>> +\tname=$1\n>> +\tdesc=$2\n>> +\toptions=\"-W $3\"\n>> +\n>> +\ttest_expect_success \"$desc\" '\n>> +\t\tgit diff $options \"$name^\" \"$name\" >\"$name.diff\"\n>> +\t'\n>> +\n>> +\ttest_expect_success ' diff applies' '\n>> +\t\ttest_when_finished \"git reset --hard\" &&\n>> +\t\tgit checkout --detach \"$name^\" &&\n>\n> With the presence of ^ there, --detach is unnecessary; it would not\n> hurt, though.\n\nRight. It's just there to make that intent clear.\n\n>> +\t\tgit apply \"$name.diff\" &&\n>> +\t\tgit diff --exit-code \"$name\"\n>\n> Even though we may know that $name.diff\" will never have a creation\n> of new paths, I'd feel safer if \"apply\" is run with \"--index\".\n\nMakes sense; the less we assume about the diff to be checked the better.\n\nThanks a lot!\n\nRené\n"},{"id":"287930","messageId":"574DED66.6050008@web.de","threadId":"42278","inReplyTo":"xmqqinxwpgfn.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2.5 1/8] t4051: rewrite, add more tests","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-31T20:00:38Z","receivedAt":"2016-05-31T20:00:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Remove the tests that checked against a fixed result and replace them\nwith more focused checks of desired properties of the created diffs.\nThat way we get more detailed and meaningful diagnostics.\n\nStore test file contents in files in a subdirectory in order to avoid\ncluttering the test script with them.\n\nUse tagged commits to store the changes to test diff -W against instead\nof using changes to the worktree. Use the worktree instead to try and\napply the generated patch in order to validate it.\n\nDocument unwanted features: trailing empty lines, too much context for\nappended functions, insufficient context at the end with -U0.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Rene Scharfe \n---\nReplacement for the first patch addressing review comments, the other\nseven patches are unchanged.\n\n t/t4051-diff-function-context.sh | 216 +++++++++++++++++++++++++--------------\n t/t4051/appended1.c | 15 +++\n t/t4051/appended2.c | 35 +++++++\n t/t4051/dummy.c | 7 ++\n t/t4051/hello.c | 21 ++++\n t/t4051/includes.c | 20 ++++\n 6 files changed, 240 insertions(+), 74 deletions(-)\n create mode 100644 t/t4051/appended1.c\n create mode 100644 t/t4051/appended2.c\n create mode 100644 t/t4051/dummy.c\n create mode 100644 t/t4051/hello.c\n create mode 100644 t/t4051/includes.c\n\ndiff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh\nindex 001d678..ca01725 100755\n--- a/t/t4051-diff-function-context.sh\n+++ b/t/t4051-diff-function-context.sh\n@@ -3,90 +3,158 @@\n test_description='diff function context'\n \n . ./test-lib.sh\n-. \"$TEST_DIRECTORY\"/diff-lib.sh\n \n+dir=\"$TEST_DIRECTORY/t4051\"\n \n-cat <<\\EOF >hello.c\n-#include \n-\n-static int a(void)\n-{\n-\t/*\n-\t * Dummy.\n-\t */\n+commit_and_tag () {\n+\ttag=$1 &&\n+\tshift &&\n+\tgit add \"$@\" &&\n+\ttest_tick &&\n+\tgit commit -m \"$tag\" &&\n+\tgit tag \"$tag\"\n }\n \n-static int hello_world(void)\n-{\n-\t/* Classic. */\n-\tprintf(\"Hello world.\\n\");\n-\n-\t/* Success! */\n-\treturn 0;\n+first_context_line () {\n+\tawk '\n+\t\tfound {print; exit}\n+\t\t/^@@/ {found = 1}\n+\t'\n }\n-static int b(void)\n-{\n-\t/*\n-\t * Dummy, too.\n-\t */\n+\n+last_context_line () {\n+\tsed -ne \\$p\n }\n \n-int main(int argc, char **argv)\n-{\n-\ta();\n-\tb();\n-\treturn hello_world();\n+check_diff () {\n+\tname=$1\n+\tdesc=$2\n+\toptions=\"-W $3\"\n+\n+\ttest_expect_success \"$desc\" '\n+\t\tgit diff $options \"$name^\" \"$name\" >\"$name.diff\"\n+\t'\n+\n+\ttest_expect_success ' diff applies' '\n+\t\ttest_when_finished \"git reset --hard\" &&\n+\t\tgit checkout --detach \"$name^\" &&\n+\t\tgit apply --index \"$name.diff\" &&\n+\t\tgit diff --exit-code \"$name\"\n+\t'\n }\n-EOF\n \n test_expect_success 'setup' '\n-\tgit add hello.c &&\n-\ttest_tick &&\n-\tgit commit -m initial &&\n-\n-\tgrep -v Classic hello.c.new &&\n-\tmv hello.c.new hello.c\n-'\n-\n-cat <<\\EOF >expected\n-diff --git a/hello.c b/hello.c\n---- a/hello.c\n-+++ b/hello.c\n-@@ -10,8 +10,7 @@ static int a(void)\n- static int hello_world(void)\n- {\n--\t/* Classic. */\n- \tprintf(\"Hello world.\\n\");\n- \n- \t/* Success! */\n- \treturn 0;\n- }\n-EOF\n-\n-test_expect_success 'diff -U0 -W' '\n-\tgit diff -U0 -W >actual &&\n-\tcompare_diff_patch actual expected\n-'\n-\n-cat <<\\EOF >expected\n-diff --git a/hello.c b/hello.c\n---- a/hello.c\n-+++ b/hello.c\n-@@ -9,9 +9,8 @@ static int a(void)\n- \n- static int hello_world(void)\n- {\n--\t/* Classic. */\n- \tprintf(\"Hello world.\\n\");\n- \n- \t/* Success! */\n- \treturn 0;\n- }\n-EOF\n-\n-test_expect_success 'diff -W' '\n-\tgit diff -W >actual &&\n-\tcompare_diff_patch actual expected\n+\tcat \"$dir/includes.c\" \"$dir/dummy.c\" \"$dir/dummy.c\" \"$dir/hello.c\" \\\n+\t\t\"$dir/dummy.c\" \"$dir/dummy.c\" >file.c &&\n+\tcommit_and_tag initial file.c &&\n+\n+\tgrep -v \"delete me from hello\" file.c.new &&\n+\tmv file.c.new file.c &&\n+\tcommit_and_tag changed_hello file.c &&\n+\n+\tgrep -v \"delete me from includes\" file.c.new &&\n+\tmv file.c.new file.c &&\n+\tcommit_and_tag changed_includes file.c &&\n+\n+\tcat \"$dir/appended1.c\" >>file.c &&\n+\tcommit_and_tag appended file.c &&\n+\n+\tcat \"$dir/appended2.c\" >>file.c &&\n+\tcommit_and_tag extended file.c &&\n+\n+\tgrep -v \"Begin of second part\" file.c.new &&\n+\tmv file.c.new file.c &&\n+\tcommit_and_tag long_common_tail file.c\n+'\n+\n+check_diff changed_hello 'changed function'\n+\n+test_expect_success ' context includes begin' '\n+\tgrep \"^ .*Begin of hello\" changed_hello.diff\n+'\n+\n+test_expect_success ' context includes end' '\n+\tgrep \"^ .*End of hello\" changed_hello.diff\n+'\n+\n+test_expect_success ' context does not include other functions' '\n+\ttest $(grep -c \"^[ +-].*Begin\" changed_hello.diff) -le 1\n+'\n+\n+test_expect_success ' context does not include preceding empty lines' '\n+\ttest \"$(first_context_line = 0;\n+\t i--) {\n+\t\tprintf(\"%d bottles of beer on the wall\\n\", i);\n+\t}\n+\n+\tprintf(\"End of first part\\n\");\ndiff --git a/t/t4051/appended2.c b/t/t4051/appended2.c\nnew file mode 100644\nindex 0000000..e651f71\n--- /dev/null\n+++ b/t/t4051/appended2.c\n@@ -0,0 +1,35 @@\n+\tprintf(\"Begin of second part\\n\");\n+\n+\t/*\n+\t * Lorem ipsum dolor sit amet, consectetuer sadipscing elitr,\n+\t * sed diam nonumy eirmod tempor invidunt ut labore et dolore\n+\t * magna aliquyam erat, sed diam voluptua. At vero eos et\n+\t * accusam et justo duo dolores et ea rebum. Stet clita kasd\n+\t * gubergren, no sea takimata sanctus est Lorem ipsum dolor\n+\t * sit amet.\n+\t *\n+\t * Lorem ipsum dolor sit amet, consectetuer sadipscing elitr,\n+\t * sed diam nonumy eirmod tempor invidunt ut labore et dolore\n+\t * magna aliquyam erat, sed diam voluptua. At vero eos et\n+\t * accusam et justo duo dolores et ea rebum. Stet clita kasd\n+\t * gubergren, no sea takimata sanctus est Lorem ipsum dolor\n+\t * sit amet.\n+\t *\n+\t * Lorem ipsum dolor sit amet, consectetuer sadipscing elitr,\n+\t * sed diam nonumy eirmod tempor invidunt ut labore et dolore\n+\t * magna aliquyam erat, sed diam voluptua. At vero eos et\n+\t * accusam et justo duo dolores et ea rebum. Stet clita kasd\n+\t * gubergren, no sea takimata sanctus est Lorem ipsum dolor\n+\t * sit amet.\n+\t *\n+\t * Lorem ipsum dolor sit amet, consectetuer sadipscing elitr,\n+\t * sed diam nonumy eirmod tempor invidunt ut labore et dolore\n+\t * magna aliquyam erat, sed diam voluptua. At vero eos et\n+\t * accusam et justo duo dolores et ea rebum. Stet clita kasd\n+\t * gubergren, no sea takimata sanctus est Lorem ipsum dolor\n+\t * sit amet.\n+\t *\n+\t */\n+\n+\treturn 0;\n+}\t// End of second part\ndiff --git a/t/t4051/dummy.c b/t/t4051/dummy.c\nnew file mode 100644\nindex 0000000..a43016e\n--- /dev/null\n+++ b/t/t4051/dummy.c\n@@ -0,0 +1,7 @@\n+\n+static int dummy(void)\t// Begin of dummy\n+{\n+\tint rc = 0;\n+\n+\treturn rc;\n+}\t// End of dummy\ndiff --git a/t/t4051/hello.c b/t/t4051/hello.c\nnew file mode 100644\nindex 0000000..63b1a1e\n--- /dev/null\n+++ b/t/t4051/hello.c\n@@ -0,0 +1,21 @@\n+\n+static void hello(void)\t// Begin of hello\n+{\n+\t/*\n+\t * Classic.\n+\t */\n+\tputchar('H');\n+\tputchar('e');\n+\tputchar('l');\n+\tputchar('l');\n+\tputchar('o');\n+\tputchar(' ');\n+\t/* delete me from hello */\n+\tputchar('w');\n+\tputchar('o');\n+\tputchar('r');\n+\tputchar('l');\n+\tputchar('d');\n+\tputchar('.');\n+\tputchar('\\n');\n+}\t// End of hello\ndiff --git a/t/t4051/includes.c b/t/t4051/includes.c\nnew file mode 100644\nindex 0000000..efc68f8\n--- /dev/null\n+++ b/t/t4051/includes.c\n@@ -0,0 +1,20 @@\n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+/* delete me from includes */\n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n+#include \n-- \n2.8.3\n"},{"id":"287932","messageId":"574DF0E8.8060908@web.de","threadId":"42278","inReplyTo":"574DED66.6050008@web.de","subject":"Re: [PATCH v2.5 1/8] t4051: rewrite, add more tests","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-05-31T20:15:36Z","receivedAt":"2016-05-31T20:15:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Interdiff between v2 and v2.5:\n- Rename $message to $tag, as that fits its purpose better, and quote it.\n- Quote $@.\n- Use the most common sed invocation from t/ for getting the last line of\n a file, for consistency.\n- Use apply --index to make sure we notice if the generated diff adds or\n deletes files unintendedly.\n---\n t/t4051-diff-function-context.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh\nindex 88a3308..b6bb04a 100755\n--- a/t/t4051-diff-function-context.sh\n+++ b/t/t4051-diff-function-context.sh\n@@ -7,12 +7,12 @@ test_description='diff function context'\n dir=\"$TEST_DIRECTORY/t4051\"\n \n commit_and_tag () {\n-\tmessage=$1 &&\n+\ttag=$1 &&\n \tshift &&\n-\tgit add $@ &&\n+\tgit add \"$@\" &&\n \ttest_tick &&\n-\tgit commit -m $message &&\n-\tgit tag $message\n+\tgit commit -m \"$tag\" &&\n+\tgit tag \"$tag\"\n }\n \n first_context_line () {\n@@ -23,7 +23,7 @@ first_context_line () {\n }\n \n last_context_line () {\n-\tsed -n '$ p'\n+\tsed -ne \\$p\n }\n \n check_diff () {\n@@ -38,7 +38,7 @@ check_diff () {\n \ttest_expect_success ' diff applies' '\n \t\ttest_when_finished \"git reset --hard\" &&\n \t\tgit checkout --detach \"$name^\" &&\n-\t\tgit apply \"$name.diff\" &&\n+\t\tgit apply --index \"$name.diff\" &&\n \t\tgit diff --exit-code \"$name\"\n \t'\n }\n"},{"id":"287936","messageId":"xmqqeg8h53co.fsf@gitster.mtv.corp.google.com","threadId":"42278","inReplyTo":"574DED66.6050008@web.de","subject":"Re: [PATCH v2.5 1/8] t4051: rewrite, add more tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-31T21:23:19Z","receivedAt":"2016-05-31T21:23:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"}]}