log -p -W showing the whole file for a patch that adds to the end?
Just noticed a curiosity:
$ git show -W 3e3ceaa58 quote.c
shows the entire file. The commit in question adds a whole new function at the end of the file. If I move that addition to just before the last function the file already had before the change and amend the commit, "show -W" would work as expected, i.e. addition of the function, with three lines before and three lines after context.
The -W feature was added by 14937c2c (diff: add option to show whole functions as context, 2011-10-09).
Quoted 4 lines · Junio C Hamano
Just noticed a curiosity:
$ git show -W 3e3ceaa58 quote.c
Quoted 8 lines
shows the entire file. The commit in question adds a whole new function at the end of the file. If I move that addition to just before the last function the file already had before the change and amend the commit, "show -W" would work as expected, i.e. addition of the function, with three lines before and three lines after context.
The -W feature was added by
14937c2c(diff: add option to show whole functions as context, 2011-10-09).
A workaround (atEnd) seems to give me a slightly better result, but I am not sure if this breaks other corner cases.
The real problem is that get_func_line() is called with (start, limit) but a hunk that adds to the end does not even enter the loop that goes back (i.e. step = -1) to find the funcline:
for (l = start; l != limit && 0 <= l && l < xe->xdf1.nrec; l += step) {
const char *rec;
long reclen = xdl_get_rec(&xe->xdf1, l, &rec);
long len = ff(rec, reclen, buf, size, xecfg->find_func_priv);because start == xe->xdf1.nrec in that case. We could instead force it to enter the loop by decrementing start when it is xe->xdf1.nrec and we are going down, but that would show two functions in the resulting hunk (one that is the existing function at the end of the file, the other that is added by the patch), which is not better than showing the patch unmodified.
-- >8 --
Subject: diff -W: do not show the whole file when punting
The -W option to "diff" family of commands allows the pre- and post- context of hunks extended to cover the previous and the next "function header line", so that the whole function that is patched can be seen in the hunk.
The helper function get_func_line() however gets confused when a hunk adds a new function at the very end, and returns -1 to signal that it did not find a suitable "function header line", i.e. the beginning of previous function. The caller then takes this signal and shows from the very beginning of the file. We end up showing the entire file, starting from the very beginning to the end of the newly added lines.
We can avoid this by using the original hunk boundary when the helper gives up.
--- xdiff/xemit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
xdiff/xemit.c +1 -1
@@ -166,7 +166,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,if (xecfg->flags &XDL_EMIT_FUNCCONTEXT ) {long fs1 =get_func_line (xe, xecfg,NULL , xch->i1,-1 );if (fs1 <0 ) - fs1 =0 ; + fs1 = xch->i1;if (fs1 < s1) { s2 -= s1 - fs1; s1 = fs1;
Am 11.05.2016 um 00:51 schrieb Junio C Hamano:
Quoted 10 lines
The helper function
get_func_line()however gets confused when a hunk adds a new function at the very end, and returns -1 to signal that it did not find a suitable "function header line", i.e. the beginning of previous function. The caller then takes this signal and shows from the very beginning of the file. We end up showing the entire file, starting from the very beginning to the end of the newly added lines.We can avoid this by using the original hunk boundary when the helper gives up.
It's a more conservative fallback in this case, but it regresses e.g. if you have a long list of includes at the top of a C file -- it won't show the full list anymore with -W.
The problem is that we are trying to access lines in the preimage that are only actually in the postimage. Simply switching to useing the postimage unconditionally is not enough -- we'd have the same problem with the complementary case of lines at the end being removed.
I wonder if using the postimage as a fallback suffices. Need to look at this more closely.
René
Am 11.05.2016 um 00:51 schrieb Junio C Hamano:
Quoted 7 lines
The helper function
get_func_line()however gets confused when a hunk adds a new function at the very end, and returns -1 to signal that it did not find a suitable "function header line", i.e. the beginning of previous function. The caller then takes this signal and shows from the very beginning of the file. We end up showing the entire file, starting from the very beginning to the end of the newly added lines.
In this case we need to look at the added lines in the post-image to see if the original context suffices. We currently only look at the pre-image. And if we have to extend the context simply search from the bottom of the pre-image. That's what the second patch does; the first one is just a small preparation.
The last three patches introduce special handling of empty lines. Before them the code for -W only distinguished between function lines and non-function lines. Not allowing empty lines between functions to extend the context with -W is most useful in the case of appended full functions -- otherwise we'd get the preceding function shown as well as it "belongs" to the empty line separating them.
And if we do that then it's easy and more consistent to stop showing empty lines trailing functions with -W (unless they're already included in the one requested with -u/-U, of course). Doing the same for grep then is only fair.
Considering empty lines as uninteresting collides with languages like Whitespace. I'm not sure -W was useful for it to begin with, though. Any other possible downsides?
NB: Comments are still not handled specially. That means they are considered to be part of the preceding function. Fixing that is not in scope for this series. (And I'm not sure it would be worth the hassle.)
diff: factor out match_func_rec()
diff: handle appended chunks better with -W
diff: ignore empty lines before added functions with -W
diff: don't include common trailing empty lines with -W
grep: don't extend context to trailing empty lines with -W
grep.c | 28 ++++++++++++++++++++++++-- xdiff/xemit.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 82 insertions(+), 9 deletions(-)
PATCH 1/5 diff: factor out match_func_rec()
Add match_func_rec(), a helper that wraps accessing a record and calling the appropriate function for checking if it contains a function line.
---
We'll use it in the next patch. A follow-up patch could inline def_ff() into it.
xdiff/xemit.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
xdiff/xemit.c +11 -4
@@ -120,6 +120,16 @@ static long def_ff(const char *rec, long len, char *buf, long sz, void *priv)return -1 ; } +static long match_func_rec (xdfile_t *xdf,xdemitconf_t const *xecfg,long ri, +char *buf,long sz) +{ +const char *rec; +long len =xdl_get_rec (xdf, ri, &rec); +if (!xecfg->find_func) +return def_ff (rec, len, buf, sz, xecfg->find_func_priv); +return xecfg->find_func (rec, len, buf, sz, xecfg->find_func_priv); +} +struct func_line {long len;char buf[80 ];
@@ -128,7 +138,6 @@ struct func_line {static long get_func_line (xdfenv_t *xe,xdemitconf_t const *xecfg,struct func_line *func_line,long start,long limit) { -find_func_t ff = xecfg->find_func ? xecfg->find_func : def_ff;long l, size, step = (start > limit) ?-1 :1 ;char *buf, dummy[1 ];
@@ -136,9 +145,7 @@ static long get_func_line(xdfenv_t *xe, xdemitconf_t const *xecfg, size = func_line ?sizeof (func_line->buf) :sizeof (dummy);for (l = start; l != limit &&0 <= l && l < xe->xdf1.nrec; l += step) { -const char *rec; -long reclen =xdl_get_rec (&xe->xdf1, l, &rec); -long len =ff (rec, reclen, buf, size, xecfg->find_func_priv); +long len =match_func_rec (&xe->xdf1, xecfg, l, buf, size);if (len >=0 ) {if (func_line) func_line->len = len;
Signature · 1 line
2.8.3
PATCH 2/5 diff: handle appended chunks better with -W
In reply to René ScharfeIf lines are added at the end of a file, diff -W shows the whole file. That's because get_func_line() only considers the pre-image and gives up if it sees a record index beyond its end.
Consider the post-image as well to see if the added lines already make up a full function. If it doesn't then search for the previous function line by starting from the bottom of the pre-image, thereby avoiding to confuse get_func_line().
Reuse the existing label called "again", as it's exactly where we need to jump to when we're done handling the pre-context, but rename it to "post_context_calculation" in order to document its new purpose better.
--- xdiff/xemit.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-)
xdiff/xemit.c +24 -3
@@ -171,7 +171,28 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb, s2 =XDL_MAX (xch->i2 - xecfg->ctxlen,0 );if (xecfg->flags &XDL_EMIT_FUNCCONTEXT ) { -long fs1 =get_func_line (xe, xecfg,NULL , xch->i1,-1 ); +long fs1, i1 = xch->i1; + +/* Appended chunk? */ +if (i1 >= xe->xdf1.nrec) { +char dummy[1 ]; + +/* +* We don't need additional context if +* a whole function was added. +*/ +if (match_func_rec (&xe->xdf2, xecfg, xch->i2, + dummy,sizeof (dummy)) >=0 ) + goto post_context_calculation; + +/* +* Otherwise get more context from the +* pre-image. +*/ + i1 = xe->xdf1.nrec -1 ; + } + + fs1 =get_func_line (xe, xecfg,NULL , i1,-1 );if (fs1 <0 ) fs1 =0 ;if (fs1 < s1) {
@@ -180,7 +201,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb, } } - again: + post_context_calculation: lctx = xecfg->ctxlen; lctx =XDL_MIN (lctx, xe->xdf1.nrec - (xche->i1 + xche->chg1)); lctx =XDL_MIN (lctx, xe->xdf2.nrec - (xche->i2 + xche->chg2));
@@ -209,7 +230,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,if (l <= e1 ||get_func_line (xe, xecfg,NULL , l, e1) <0 ) { xche = xche->next; - goto again; + goto post_context_calculation; } } }
Signature · 1 line
2.8.3
PATCH 3/5 diff: ignore empty lines before added functions with -W
In reply to René ScharfeIf a new function and a preceding empty line is appended, diff -W shows the previous function in full in order to provide context for that empty line. In most languages empty lines between sections are not interesting in and off themselves and showing a whole extra function for them is not what we want.
Skip empty lines when checking of the appended chunk starts with a function line, thereby avoiding to extend the context just for them.
--- xdiff/xemit.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)
xdiff/xemit.c +20 -2
@@ -155,6 +155,18 @@ static long get_func_line(xdfenv_t *xe, xdemitconf_t const *xecfg,return -1 ; } +static int is_empty_rec (xdfile_t *xdf,xdemitconf_t const *xecfg,long ri) +{ +const char *rec; +long len =xdl_get_rec (xdf, ri, &rec); + +while (len >0 &&isspace (*rec)) { + rec++; + len--; + } +return !len; +} +int xdl_emit_diff (xdfenv_t *xe,xdchange_t *xscr,xdemitcb_t *ecb,xdemitconf_t const *xecfg) {long s1, s2, e1, e2, lctx;
@@ -176,12 +188,18 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,/* Appended chunk? */ if (i1 >= xe->xdf1.nrec) {char dummy[1 ]; +long i2 = xch->i2;/* * We don't need additional context if -* a whole function was added. +* a whole function was added, possibly +* starting with empty lines. */ -if (match_func_rec (&xe->xdf2, xecfg, xch->i2, +while (i2 < xe->xdf2.nrec && +is_empty_rec (&xe->xdf2, xecfg, i2)) + i2++; +if (i2 < xe->xdf2.nrec && +match_func_rec (&xe->xdf2, xecfg, i2, dummy,sizeof (dummy)) >=0 ) goto post_context_calculation;
Signature · 1 line
2.8.3
PATCH 4/5 diff: don't include common trailing empty lines with -W
In reply to René ScharfeEmpty lines between functions are shown by diff -W, as it considers them to be part of the function preceding them. They are not interesting in most languages. The previous patch stopped showing them in the special case of a function added at the end of a file.
Stop extending context to those empty lines by skipping back over them from the start of the next function.
--- xdiff/xemit.c | 3 +++ 1 file changed, 3 insertions(+)
xdiff/xemit.c +3 -0
@@ -231,6 +231,9 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,long fe1 =get_func_line (xe, xecfg,NULL , xche->i1 + xche->chg1, xe->xdf1.nrec); +while (fe1 >0 && +is_empty_rec (&xe->xdf1, xecfg, fe1 -1 )) + fe1--;if (fe1 <0 ) fe1 = xe->xdf1.nrec;if (fe1 > e1) {
Signature · 1 line
2.8.3
PATCH 5/5 grep: don't extend context to trailing empty lines with -W
In reply to René ScharfeEmpty lines between functions are shown by grep -W, as it considers them to be part of the function preceding them. They are not interesting in most languages. The previous patches stopped showing them for diff -W.
Stop showing empty lines trailing a function with grep -W. Grep scans the lines of a buffer from top to bottom and prints matching lines immediately. Thus we need to peek ahead in order to determine if an empty line is part of a function body and worth showing or not.
Remember how far ahead we peeked in order to avoid having to do so repeatedly when handling multiple consecutive empty lines.
--- grep.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)
grep.c +26 -2
@@ -1396,9 +1396,17 @@ static int fill_textconv_grep(struct userdiff_driver *driver,return 0 ; } +static int is_empty_line (const char *bol,const char *eol) +{ +while (bol < eol &&isspace (*bol)) + bol++; +return bol == eol; +} +static int grep_source_1 (struct grep_opt *opt,struct grep_source *gs,int collect_hits) {char *bol; +char *peek_bol =NULL ;unsigned long left;unsigned lno =1 ;unsigned last_hit =0 ;
@@ -1543,8 +1551,24 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle show_function =1 ; goto next_line; } -if (show_function &&match_funcname (opt, gs, bol, eol)) - show_function =0 ; +if (show_function && (!peek_bol || peek_bol < bol)) { +unsigned long peek_left = left; +char *peek_eol = eol; + +/* +* Trailing empty lines are not interesting. +* Peek past them to see if they belong to the +* body of the current function. +*/ + peek_bol = bol; +while (is_empty_line (peek_bol, peek_eol)) { + peek_bol = peek_eol +1 ; + peek_eol =end_of_line (peek_bol, &peek_left); + } + +if (match_funcname (opt, gs, peek_bol, peek_eol)) + show_function =0 ; + }if (show_function || (last_hit && lno <= last_hit + opt->post_context)) {/* If the last hit is within the post context,
Signature · 1 line
2.8.3
Quoted 9 lines · René Scharfe
diff: factor out
match_func_rec()
diff: handle appended chunks better with -W
diff: ignore empty lines before added functions with -W
diff: don't include common trailing empty lines with -W
grep: don't extend context to trailing empty lines with -Wgrep.c | 28 ++++++++++++++++++++++++-- xdiff/xemit.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 82 insertions(+), 9 deletions(-)
Thanks. The flow of reasoning in this cover letter was written very well and I think I can agree with all of them.
It is curious that this much behaviour change does not need any changes in the test scripts. We do not have sufficient coverage, perhaps?
Am 24.05.2016 um 20:16 schrieb Junio C Hamano:
Quoted 8 lines
Quoted 6 lines · René Scharfe
diff: factor out
match_func_rec()
diff: handle appended chunks better with -W
diff: ignore empty lines before added functions with -W
diff: don't include common trailing empty lines with -W
grep: don't extend context to trailing empty lines with -W
Quoted 7 lines
Quoted 3 lines
grep.c | 28 ++++++++++++++++++++++++-- xdiff/xemit.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 82 insertions(+), 9 deletions(-)It is curious that this much behaviour change does not need any changes in the test scripts. We do not have sufficient coverage, perhaps?
Well, -W is not tested at all. I'll include some in the next round. No need to hurry, it's too late to land in 2.9.0 anyway.
René
Quoted 13 lines · René Scharfe
Am 24.05.2016 um 20:16 schrieb Junio C Hamano:
Quoted 11 lines
Quoted 9 lines · René Scharfe
diff: factor out
match_func_rec()
diff: handle appended chunks better with -W
diff: ignore empty lines before added functions with -W
diff: don't include common trailing empty lines with -W
grep: don't extend context to trailing empty lines with -Wgrep.c | 28 ++++++++++++++++++++++++-- xdiff/xemit.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 82 insertions(+), 9 deletions(-)
Quoted 6 lines
Quoted 3 lines
It is curious that this much behaviour change does not need any changes in the test scripts. We do not have sufficient coverage, perhaps?
Well, -W is not tested at all. I'll include some in the next round. No need to hurry, it's too late to land in 2.9.0 anyway.
True.
I'd say that these patches are fine as they are, and follow-up patch for adding -W tests (instead of rerolling them) is sufficient, though.
Am 26.05.2016 um 19:05 schrieb Junio C Hamano:
Quoted 3 lines
I'd say that these patches are fine as they are, and follow-up patch for adding -W tests (instead of rerolling them) is sufficient, though.
Patch 3 needs two small updates to address the char signedness issue found by Ramsay and to get rid of an unused function parameter, and patch 4 needs a small change as a result of the latter as well. Shouldn't be long before tests are done..
René
Am 21.05.2016 um 20:42 schrieb René Scharfe:
Quoted 41 lines
Am 11.05.2016 um 00:51 schrieb Junio C Hamano:
Quoted 7 lines
The helper function
get_func_line()however gets confused when a hunk adds a new function at the very end, and returns -1 to signal that it did not find a suitable "function header line", i.e. the beginning of previous function. The caller then takes this signal and shows from the very beginning of the file. We end up showing the entire file, starting from the very beginning to the end of the newly added lines.In this case we need to look at the added lines in the post-image to see if the original context suffices. We currently only look at the pre-image. And if we have to extend the context simply search from the bottom of the pre-image. That's what the second patch does; the first one is just a small preparation.
The last three patches introduce special handling of empty lines. Before them the code for -W only distinguished between function lines and non-function lines. Not allowing empty lines between functions to extend the context with -W is most useful in the case of appended full functions -- otherwise we'd get the preceding function shown as well as it "belongs" to the empty line separating them.
And if we do that then it's easy and more consistent to stop showing empty lines trailing functions with -W (unless they're already included in the one requested with -u/-U, of course). Doing the same for grep then is only fair.
Considering empty lines as uninteresting collides with languages like Whitespace. I'm not sure -W was useful for it to begin with, though. Any other possible downsides?
NB: Comments are still not handled specially. That means they are considered to be part of the preceding function. Fixing that is not in scope for this series. (And I'm not sure it would be worth the hassle.)
diff: factor out
match_func_rec()
diff: handle appended chunks better with -W
diff: ignore empty lines before added functions with -W
diff: don't include common trailing empty lines with -W
grep: don't extend context to trailing empty lines with -W
Quoted 3 lines
grep.c | 28 ++++++++++++++++++++++++-- xdiff/xemit.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 82 insertions(+), 9 deletions(-)
And here's the second round, this time with tests, a fix for a signedness issue found by Ramsay, a small cleanup of the new function is_empty_rec() and a new fix for the case of using git diff -W -U0. An interdiff for the files touched by the original series is at the bottom.
t4051: rewrite, add more tests
xdiff: factor out match_func_rec()
xdiff: handle appended chunks better with -W
xdiff: ignore empty lines before added functions with -W
xdiff: -W: don't include common trailing empty lines in context
xdiff: don't trim common tail with -W
t7810: add test for grep -W and trailing empty context lines
grep: -W: don't extend context to trailing empty lines
grep.c | 28 ++++- t/t4051-diff-function-context.sh | 216 +++++++++++++++++++++++++-------------- t/t4051/appended1.c | 15 +++ t/t4051/appended2.c | 35 +++++++ t/t4051/dummy.c | 7 ++ t/t4051/hello.c | 21 ++++ t/t4051/includes.c | 20 ++++ t/t7810-grep.sh | 19 +++- xdiff-interface.c | 10 +- xdiff/xemit.c | 62 +++++++++-- 10 files changed, 341 insertions(+), 92 deletions(-) create mode 100644 t/t4051/appended1.c create mode 100644 t/t4051/appended2.c create mode 100644 t/t4051/dummy.c create mode 100644 t/t4051/hello.c create mode 100644 t/t4051/includes.c
xdiff/xemit.c +4 -5
@@ -155,12 +155,12 @@ static long get_func_line(xdfenv_t *xe, xdemitconf_t const *xecfg,return -1 ; } -static int is_empty_rec (xdfile_t *xdf,xdemitconf_t const *xecfg,long ri) +static int is_empty_rec (xdfile_t *xdf,long ri) {const char *rec;long len =xdl_get_rec (xdf, ri, &rec); -while (len >0 &&isspace (*rec)) { +while (len >0 &&XDL_ISSPACE (*rec)) { rec++; len--; }
@@ -196,7 +196,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,* starting with empty lines. */ while (i2 < xe->xdf2.nrec && -is_empty_rec (&xe->xdf2, xecfg, i2)) +is_empty_rec (&xe->xdf2, i2)) i2++;if (i2 < xe->xdf2.nrec && match_func_rec(&xe->xdf2, xecfg, i2,
@@ -231,8 +231,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,long fe1 =get_func_line (xe, xecfg,NULL , xche->i1 + xche->chg1, xe->xdf1.nrec); -while (fe1 >0 && -is_empty_rec (&xe->xdf1, xecfg, fe1 -1 )) +while (fe1 >0 &&is_empty_rec (&xe->xdf1, fe1 -1 )) fe1--;if (fe1 <0 ) fe1 = xe->xdf1.nrec;
PATCH v2 1/8 t4051: rewrite, add more tests
Remove the tests that checked diff -W output against a fixed expected result and replace them with more focused checks of desired properties of the created diffs. That way we get more detailed and meaningful diagnostics.
Store test file contents in files in a subdirectory in order to avoid cluttering the test script with them.
Use tagged commits to store the changes to test diff -W against instead of using changes to the worktree. Use the worktree instead to try and apply the generated patch in order to validate it.
Document unwanted features: trailing empty lines, too much context for appended functions, insufficient context at the end with -U0.
--- t/t4051-diff-function-context.sh | 216 +++++++++++++++++++++++++-------------- t/t4051/appended1.c | 15 +++ t/t4051/appended2.c | 35 +++++++ t/t4051/dummy.c | 7 ++ t/t4051/hello.c | 21 ++++ t/t4051/includes.c | 20 ++++ 6 files changed, 240 insertions(+), 74 deletions(-) create mode 100644 t/t4051/appended1.c create mode 100644 t/t4051/appended2.c create mode 100644 t/t4051/dummy.c create mode 100644 t/t4051/hello.c create mode 100644 t/t4051/includes.c
t/t4051-diff-function-context.sh +142 -74
@@ -3,90 +3,158 @@ test_description='diff function context' . ./test-lib.sh -. "$TEST_DIRECTORY"/diff-lib.sh +dir="$TEST_DIRECTORY/t4051" -cat <<\EOF >hello.c -#include <stdio.h> - -static int a(void) -{ - /* - * Dummy. - */ +commit_and_tag () { + message=$1 && + shift && + git add $@ && + test_tick && + git commit -m $message && + git tag $message } -static int hello_world(void) -{ - /* Classic. */ - printf("Hello world.\n"); - - /* Success! */ - return 0; +first_context_line () { + awk ' + found {print; exit} + /^@@/ {found = 1} + ' } -static int b(void) -{ - /* - * Dummy, too. - */ + +last_context_line () { + sed -n '$ p' } -int main(int argc, char **argv) -{ - a(); - b(); - return hello_world(); +check_diff () { + name=$1 + desc=$2 + options="-W $3" + + test_expect_success "$desc" ' + git diff $options "$name^" "$name" >"$name.diff" + ' + + test_expect_success ' diff applies' ' + test_when_finished "git reset --hard" && + git checkout --detach "$name^" && + git apply "$name.diff" && + git diff --exit-code "$name" + ' } -EOF test_expect_success 'setup' ' - git add hello.c && - test_tick && - git commit -m initial && - - grep -v Classic <hello.c >hello.c.new && - mv hello.c.new hello.c -' - -cat <<\EOF >expected -diff --git a/hello.c b/hello.c ---- a/hello.c -+++ b/hello.c -@@ -10,8 +10,7 @@ static int a(void) - static int hello_world(void) - { -- /* Classic. */ - printf("Hello world.\n"); - - /* Success! */ - return 0; - } -EOF - -test_expect_success 'diff -U0 -W' ' - git diff -U0 -W >actual && - compare_diff_patch actual expected -' - -cat <<\EOF >expected -diff --git a/hello.c b/hello.c ---- a/hello.c -+++ b/hello.c -@@ -9,9 +9,8 @@ static int a(void) - - static int hello_world(void) - { -- /* Classic. */ - printf("Hello world.\n"); - - /* Success! */ - return 0; - } -EOF - -test_expect_success 'diff -W' ' - git diff -W >actual && - compare_diff_patch actual expected + cat "$dir/includes.c" "$dir/dummy.c" "$dir/dummy.c" "$dir/hello.c" \ + "$dir/dummy.c" "$dir/dummy.c" >file.c && + commit_and_tag initial file.c && + + grep -v "delete me from hello" <file.c >file.c.new && + mv file.c.new file.c && + commit_and_tag changed_hello file.c && + + grep -v "delete me from includes" <file.c >file.c.new && + mv file.c.new file.c && + commit_and_tag changed_includes file.c && + + cat "$dir/appended1.c" >>file.c && + commit_and_tag appended file.c && + + cat "$dir/appended2.c" >>file.c && + commit_and_tag extended file.c && + + grep -v "Begin of second part" <file.c >file.c.new && + mv file.c.new file.c && + commit_and_tag long_common_tail file.c +' + +check_diff changed_hello 'changed function' + +test_expect_success ' context includes begin' ' + grep "^ .*Begin of hello" changed_hello.diff +' + +test_expect_success ' context includes end' ' + grep "^ .*End of hello" changed_hello.diff +' + +test_expect_success ' context does not include other functions' ' + test $(grep -c "^[ +-].*Begin" changed_hello.diff) -le 1 +' + +test_expect_success ' context does not include preceding empty lines' ' + test "$(first_context_line <changed_hello.diff)" != " " +' + +test_expect_failure ' context does not include trailing empty lines' ' + test "$(last_context_line <changed_hello.diff)" != " " +' + +check_diff changed_includes 'changed includes' + +test_expect_success ' context includes begin' ' + grep "^ .*Begin.h" changed_includes.diff +' + +test_expect_success ' context includes end' ' + grep "^ .*End.h" changed_includes.diff +' + +test_expect_success ' context does not include other functions' ' + test $(grep -c "^[ +-].*Begin" changed_includes.diff) -le 1 +' + +test_expect_failure ' context does not include trailing empty lines' ' + test "$(last_context_line <changed_includes.diff)" != " " +' + +check_diff appended 'appended function' + +test_expect_success ' context includes begin' ' + grep "^[+].*Begin of first part" appended.diff +' + +test_expect_success ' context includes end' ' + grep "^[+].*End of first part" appended.diff +' + +test_expect_failure ' context does not include other functions' ' + test $(grep -c "^[ +-].*Begin" appended.diff) -le 1 +' + +check_diff extended 'appended function part' + +test_expect_success ' context includes begin' ' + grep "^ .*Begin of first part" extended.diff +' + +test_expect_success ' context includes end' ' + grep "^[+].*End of second part" extended.diff +' + +test_expect_failure ' context does not include other functions' ' + test $(grep -c "^[ +-].*Begin" extended.diff) -le 2 +' + +test_expect_success ' context does not include preceding empty lines' ' + test "$(first_context_line <extended.diff)" != " " +' + +check_diff long_common_tail 'change with long common tail and no context' -U0 + +test_expect_success ' context includes begin' ' + grep "^ .*Begin of first part" long_common_tail.diff +' + +test_expect_failure ' context includes end' ' + grep "^ .*End of second part" long_common_tail.diff +' + +test_expect_success ' context does not include other functions' ' + test $(grep -c "^[ +-].*Begin" long_common_tail.diff) -le 2 +' + +test_expect_success ' context does not include preceding empty lines' ' + test "$(first_context_line <long_common_tail.diff.diff)" != " " ' test_done
t/t4051/appended1.c new file +15 -0
@@ -0,0 +1,15 @@ + +int appended (void )// Begin of first part +{ +int i; +char *s ="a string" ; + +printf ("%s\n" , s); + +for (i =99 ; + i >=0 ; + i--) { +printf ("%d bottles of beer on the wall\n" , i); + } + +printf ("End of first part\n" );
t/t4051/appended2.c new file +35 -0
@@ -0,0 +1,35 @@ +printf ("Begin of second part\n" ); + +/* +* Lorem ipsum dolor sit amet, consectetuer sadipscing elitr, +* sed diam nonumy eirmod tempor invidunt ut labore et dolore +* magna aliquyam erat, sed diam voluptua. At vero eos et +* accusam et justo duo dolores et ea rebum. Stet clita kasd +* gubergren, no sea takimata sanctus est Lorem ipsum dolor +* sit amet. +* +* Lorem ipsum dolor sit amet, consectetuer sadipscing elitr, +* sed diam nonumy eirmod tempor invidunt ut labore et dolore +* magna aliquyam erat, sed diam voluptua. At vero eos et +* accusam et justo duo dolores et ea rebum. Stet clita kasd +* gubergren, no sea takimata sanctus est Lorem ipsum dolor +* sit amet. +* +* Lorem ipsum dolor sit amet, consectetuer sadipscing elitr, +* sed diam nonumy eirmod tempor invidunt ut labore et dolore +* magna aliquyam erat, sed diam voluptua. At vero eos et +* accusam et justo duo dolores et ea rebum. Stet clita kasd +* gubergren, no sea takimata sanctus est Lorem ipsum dolor +* sit amet. +* +* Lorem ipsum dolor sit amet, consectetuer sadipscing elitr, +* sed diam nonumy eirmod tempor invidunt ut labore et dolore +* magna aliquyam erat, sed diam voluptua. At vero eos et +* accusam et justo duo dolores et ea rebum. Stet clita kasd +* gubergren, no sea takimata sanctus est Lorem ipsum dolor +* sit amet. +* +*/ + +return 0 ; +}// End of second part
t/t4051/dummy.c new file +7 -0
@@ -0,0 +1,7 @@ + +static int dummy (void )// Begin of dummy +{ +int rc =0 ; + +return rc; +}// End of dummy
t/t4051/hello.c new file +21 -0
@@ -0,0 +1,21 @@ + +static void hello (void )// Begin of hello +{ +/* +* Classic. +*/ +putchar ('H' ); +putchar ('e' ); +putchar ('l' ); +putchar ('l' ); +putchar ('o' ); +putchar (' ' ); +/* delete me from hello */ +putchar ('w' ); +putchar ('o' ); +putchar ('r' ); +putchar ('l' ); +putchar ('d' ); +putchar ('.' ); +putchar ('\n' ); +}// End of hello
t/t4051/includes.c new file +20 -0
@@ -0,0 +1,20 @@ +#include <Begin.h> +#include <unistd.h> +#include <stdio.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <stddef.h> +#include <stdlib.h> +#include <stdarg.h> +/* delete me from includes */ +#include <string.h> +#include <sys/types.h> +#include <dirent.h> +#include <sys/time.h> +#include <time.h> +#include <signal.h> +#include <assert.h> +#include <regex.h> +#include <utime.h> +#include <syslog.h> +#include <End.h>
Signature · 1 line
2.8.3
Re: [PATCH v2 1/8] t4051: rewrite, add more tests
t/t4051-diff-function-context.sh line 12
Quoted 4 lines · René Scharfe
t/t4051-diff-function-context.sh +4
-0+commit_and_tag () { + message=$1 && + shift && + git add $@ &&
Lack of dq around $@ makes me wonder if there is something funny going on (looking at the callers, there isn't, so we'd better quote it to avoid wasting time, I think).
t/t4051-diff-function-context.sh line 16
Quoted 4 lines
t/t4051-diff-function-context.sh +3
-0+ test_tick && + git commit -m $message && + git tag $message }
The use of $message as the sole argument to "git tag" makes the readers guess that it must be a single token without any funny character, so the readers would probably do not waste too much time wondreing if the lack of dq around $message in the last two is problematic.
t/t4051-diff-function-context.sh line 27
Quoted 3 lines
t/t4051-diff-function-context.sh +2
-0+last_context_line () { + sed -n '$ p' }
I have a vague recollection that some implementations of sed are unhappy to see that space between the address and the operation; I'd feel safer without it.
t/t4051-diff-function-context.sh line 40
Quoted 12 lines
t/t4051-diff-function-context.sh +12
-0+check_diff () { + name=$1 + desc=$2 + options="-W $3" + + test_expect_success "$desc" ' + git diff $options "$name^" "$name" >"$name.diff" + ' + + test_expect_success ' diff applies' ' + test_when_finished "git reset --hard" && + git checkout --detach "$name^" &&
With the presence of ^ there, --detach is unnecessary; it would not hurt, though.
t/t4051-diff-function-context.sh line 42
Quoted 2 lines
t/t4051-diff-function-context.sh +2
-0+ git apply "$name.diff" && + git diff --exit-code "$name"
Even though we may know that $name.diff" will never have a creation of new paths, I'd feel safer if "apply" is run with "--index".
Thanks.
Re: [PATCH v2 1/8] t4051: rewrite, add more tests
Am 30.05.2016 um 01:55 schrieb Junio C Hamano:
Quoted 10 lines
t/t4051-diff-function-context.sh line 12
Quoted 4 lines · René Scharfe
t/t4051-diff-function-context.sh +4
-0+commit_and_tag () { + message=$1 && + shift && + git add $@ &&Lack of dq around $@ makes me wonder if there is something funny going on (looking at the callers, there isn't, so we'd better quote it to avoid wasting time, I think).
OK.
Quoted 10 lines
t/t4051-diff-function-context.sh line 16
Quoted 4 lines
t/t4051-diff-function-context.sh +3
-0+ test_tick && + git commit -m $message && + git tag $message }The use of $message as the sole argument to "git tag" makes the readers guess that it must be a single token without any funny character, so the readers would probably do not waste too much time wondreing if the lack of dq around $message in the last two is problematic.
Well, let's call it $tag; $message is a bit misleading here. The saved letters can be invested in quotes. ;)
Quoted 7 lines
t/t4051-diff-function-context.sh line 27
Quoted 3 lines
t/t4051-diff-function-context.sh +2
-0+last_context_line () { + sed -n '$ p' }I have a vague recollection that some implementations of sed are unhappy to see that space between the address and the operation; I'd feel safer without it.
Indeed most sed calls in t/ have no space there (found counter-examples only in annotate-tests.sh, t4201-shortlog.sh, t9824-git-p4-git-lfs.sh).
Quoted 7 lines
Quoted 7 lines
t/t4051-diff-function-context.sh +7
-0+check_diff () { + name=$1 + desc=$2 + options="-W $3" + + test_expect_success "$desc" ' + git diff $options "$name^" "$name" >"$name.diff"
Quoted 8 lines
t/t4051-diff-function-context.sh line 40
Quoted 5 lines
t/t4051-diff-function-context.sh +5
-0+ ' + + test_expect_success ' diff applies' ' + test_when_finished "git reset --hard" && + git checkout --detach "$name^" &&With the presence of ^ there, --detach is unnecessary; it would not hurt, though.
Right. It's just there to make that intent clear.
Quoted 5 lines
t/t4051-diff-function-context.sh line 42
Quoted 2 lines
t/t4051-diff-function-context.sh +2
-0+ git apply "$name.diff" && + git diff --exit-code "$name"Even though we may know that $name.diff" will never have a creation of new paths, I'd feel safer if "apply" is run with "--index".
Makes sense; the less we assume about the diff to be checked the better.
Thanks a lot!
René
PATCH 1/8 t4051: rewrite, add more tests
In reply to Junio C HamanoRemove the tests that checked against a fixed result and replace them with more focused checks of desired properties of the created diffs. That way we get more detailed and meaningful diagnostics.
Store test file contents in files in a subdirectory in order to avoid cluttering the test script with them.
Use tagged commits to store the changes to test diff -W against instead of using changes to the worktree. Use the worktree instead to try and apply the generated patch in order to validate it.
Document unwanted features: trailing empty lines, too much context for appended functions, insufficient context at the end with -U0.
---
Replacement for the first patch addressing review comments, the other seven patches are unchanged.
t/t4051-diff-function-context.sh | 216 +++++++++++++++++++++++++-------------- t/t4051/appended1.c | 15 +++ t/t4051/appended2.c | 35 +++++++ t/t4051/dummy.c | 7 ++ t/t4051/hello.c | 21 ++++ t/t4051/includes.c | 20 ++++ 6 files changed, 240 insertions(+), 74 deletions(-) create mode 100644 t/t4051/appended1.c create mode 100644 t/t4051/appended2.c create mode 100644 t/t4051/dummy.c create mode 100644 t/t4051/hello.c create mode 100644 t/t4051/includes.c
t/t4051-diff-function-context.sh +142 -74
@@ -3,90 +3,158 @@ test_description='diff function context' . ./test-lib.sh -. "$TEST_DIRECTORY"/diff-lib.sh +dir="$TEST_DIRECTORY/t4051" -cat <<\EOF >hello.c -#include <stdio.h> - -static int a(void) -{ - /* - * Dummy. - */ +commit_and_tag () { + tag=$1 && + shift && + git add "$@" && + test_tick && + git commit -m "$tag" && + git tag "$tag" } -static int hello_world(void) -{ - /* Classic. */ - printf("Hello world.\n"); - - /* Success! */ - return 0; +first_context_line () { + awk ' + found {print; exit} + /^@@/ {found = 1} + ' } -static int b(void) -{ - /* - * Dummy, too. - */ + +last_context_line () { + sed -ne \$p } -int main(int argc, char **argv) -{ - a(); - b(); - return hello_world(); +check_diff () { + name=$1 + desc=$2 + options="-W $3" + + test_expect_success "$desc" ' + git diff $options "$name^" "$name" >"$name.diff" + ' + + test_expect_success ' diff applies' ' + test_when_finished "git reset --hard" && + git checkout --detach "$name^" && + git apply --index "$name.diff" && + git diff --exit-code "$name" + ' } -EOF test_expect_success 'setup' ' - git add hello.c && - test_tick && - git commit -m initial && - - grep -v Classic <hello.c >hello.c.new && - mv hello.c.new hello.c -' - -cat <<\EOF >expected -diff --git a/hello.c b/hello.c ---- a/hello.c -+++ b/hello.c -@@ -10,8 +10,7 @@ static int a(void) - static int hello_world(void) - { -- /* Classic. */ - printf("Hello world.\n"); - - /* Success! */ - return 0; - } -EOF - -test_expect_success 'diff -U0 -W' ' - git diff -U0 -W >actual && - compare_diff_patch actual expected -' - -cat <<\EOF >expected -diff --git a/hello.c b/hello.c ---- a/hello.c -+++ b/hello.c -@@ -9,9 +9,8 @@ static int a(void) - - static int hello_world(void) - { -- /* Classic. */ - printf("Hello world.\n"); - - /* Success! */ - return 0; - } -EOF - -test_expect_success 'diff -W' ' - git diff -W >actual && - compare_diff_patch actual expected + cat "$dir/includes.c" "$dir/dummy.c" "$dir/dummy.c" "$dir/hello.c" \ + "$dir/dummy.c" "$dir/dummy.c" >file.c && + commit_and_tag initial file.c && + + grep -v "delete me from hello" <file.c >file.c.new && + mv file.c.new file.c && + commit_and_tag changed_hello file.c && + + grep -v "delete me from includes" <file.c >file.c.new && + mv file.c.new file.c && + commit_and_tag changed_includes file.c && + + cat "$dir/appended1.c" >>file.c && + commit_and_tag appended file.c && + + cat "$dir/appended2.c" >>file.c && + commit_and_tag extended file.c && + + grep -v "Begin of second part" <file.c >file.c.new && + mv file.c.new file.c && + commit_and_tag long_common_tail file.c +' + +check_diff changed_hello 'changed function' + +test_expect_success ' context includes begin' ' + grep "^ .*Begin of hello" changed_hello.diff +' + +test_expect_success ' context includes end' ' + grep "^ .*End of hello" changed_hello.diff +' + +test_expect_success ' context does not include other functions' ' + test $(grep -c "^[ +-].*Begin" changed_hello.diff) -le 1 +' + +test_expect_success ' context does not include preceding empty lines' ' + test "$(first_context_line <changed_hello.diff)" != " " +' + +test_expect_failure ' context does not include trailing empty lines' ' + test "$(last_context_line <changed_hello.diff)" != " " +' + +check_diff changed_includes 'changed includes' + +test_expect_success ' context includes begin' ' + grep "^ .*Begin.h" changed_includes.diff +' + +test_expect_success ' context includes end' ' + grep "^ .*End.h" changed_includes.diff +' + +test_expect_success ' context does not include other functions' ' + test $(grep -c "^[ +-].*Begin" changed_includes.diff) -le 1 +' + +test_expect_failure ' context does not include trailing empty lines' ' + test "$(last_context_line <changed_includes.diff)" != " " +' + +check_diff appended 'appended function' + +test_expect_success ' context includes begin' ' + grep "^[+].*Begin of first part" appended.diff +' + +test_expect_success ' context includes end' ' + grep "^[+].*End of first part" appended.diff +' + +test_expect_failure ' context does not include other functions' ' + test $(grep -c "^[ +-].*Begin" appended.diff) -le 1 +' + +check_diff extended 'appended function part' + +test_expect_success ' context includes begin' ' + grep "^ .*Begin of first part" extended.diff +' + +test_expect_success ' context includes end' ' + grep "^[+].*End of second part" extended.diff +' + +test_expect_failure ' context does not include other functions' ' + test $(grep -c "^[ +-].*Begin" extended.diff) -le 2 +' + +test_expect_success ' context does not include preceding empty lines' ' + test "$(first_context_line <extended.diff)" != " " +' + +check_diff long_common_tail 'change with long common tail and no context' -U0 + +test_expect_success ' context includes begin' ' + grep "^ .*Begin of first part" long_common_tail.diff +' + +test_expect_failure ' context includes end' ' + grep "^ .*End of second part" long_common_tail.diff +' + +test_expect_success ' context does not include other functions' ' + test $(grep -c "^[ +-].*Begin" long_common_tail.diff) -le 2 +' + +test_expect_success ' context does not include preceding empty lines' ' + test "$(first_context_line <long_common_tail.diff.diff)" != " " ' test_done
t/t4051/appended1.c new file +15 -0
@@ -0,0 +1,15 @@ + +int appended (void )// Begin of first part +{ +int i; +char *s ="a string" ; + +printf ("%s\n" , s); + +for (i =99 ; + i >=0 ; + i--) { +printf ("%d bottles of beer on the wall\n" , i); + } + +printf ("End of first part\n" );
t/t4051/appended2.c new file +35 -0
@@ -0,0 +1,35 @@ +printf ("Begin of second part\n" ); + +/* +* Lorem ipsum dolor sit amet, consectetuer sadipscing elitr, +* sed diam nonumy eirmod tempor invidunt ut labore et dolore +* magna aliquyam erat, sed diam voluptua. At vero eos et +* accusam et justo duo dolores et ea rebum. Stet clita kasd +* gubergren, no sea takimata sanctus est Lorem ipsum dolor +* sit amet. +* +* Lorem ipsum dolor sit amet, consectetuer sadipscing elitr, +* sed diam nonumy eirmod tempor invidunt ut labore et dolore +* magna aliquyam erat, sed diam voluptua. At vero eos et +* accusam et justo duo dolores et ea rebum. Stet clita kasd +* gubergren, no sea takimata sanctus est Lorem ipsum dolor +* sit amet. +* +* Lorem ipsum dolor sit amet, consectetuer sadipscing elitr, +* sed diam nonumy eirmod tempor invidunt ut labore et dolore +* magna aliquyam erat, sed diam voluptua. At vero eos et +* accusam et justo duo dolores et ea rebum. Stet clita kasd +* gubergren, no sea takimata sanctus est Lorem ipsum dolor +* sit amet. +* +* Lorem ipsum dolor sit amet, consectetuer sadipscing elitr, +* sed diam nonumy eirmod tempor invidunt ut labore et dolore +* magna aliquyam erat, sed diam voluptua. At vero eos et +* accusam et justo duo dolores et ea rebum. Stet clita kasd +* gubergren, no sea takimata sanctus est Lorem ipsum dolor +* sit amet. +* +*/ + +return 0 ; +}// End of second part
t/t4051/dummy.c new file +7 -0
@@ -0,0 +1,7 @@ + +static int dummy (void )// Begin of dummy +{ +int rc =0 ; + +return rc; +}// End of dummy
t/t4051/hello.c new file +21 -0
@@ -0,0 +1,21 @@ + +static void hello (void )// Begin of hello +{ +/* +* Classic. +*/ +putchar ('H' ); +putchar ('e' ); +putchar ('l' ); +putchar ('l' ); +putchar ('o' ); +putchar (' ' ); +/* delete me from hello */ +putchar ('w' ); +putchar ('o' ); +putchar ('r' ); +putchar ('l' ); +putchar ('d' ); +putchar ('.' ); +putchar ('\n' ); +}// End of hello
t/t4051/includes.c new file +20 -0
@@ -0,0 +1,20 @@ +#include <Begin.h> +#include <unistd.h> +#include <stdio.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <stddef.h> +#include <stdlib.h> +#include <stdarg.h> +/* delete me from includes */ +#include <string.h> +#include <sys/types.h> +#include <dirent.h> +#include <sys/time.h> +#include <time.h> +#include <signal.h> +#include <assert.h> +#include <regex.h> +#include <utime.h> +#include <syslog.h> +#include <End.h>
Signature · 1 line
2.8.3
Re: [PATCH v2.5 1/8] t4051: rewrite, add more tests
Interdiff between v2 and v2.5:
- Rename $message to $tag, as that fits its purpose better, and quote it.
- Quote $@.
- Use the most common sed invocation from t/ for getting the last line of a file, for consistency.
- Use apply --index to make sure we notice if the generated diff adds or deletes files unintendedly.
--- t/t4051-diff-function-context.sh | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
t/t4051-diff-function-context.sh +6 -6
@@ -7,12 +7,12 @@ test_description='diff function context' dir="$TEST_DIRECTORY/t4051" commit_and_tag () { - message=$1 && + tag=$1 && shift && - git add $@ && + git add "$@" && test_tick && - git commit -m $message && - git tag $message + git commit -m "$tag" && + git tag "$tag" } first_context_line () {
@@ -23,7 +23,7 @@ first_context_line () { } last_context_line () { - sed -n '$ p' + sed -ne \$p } check_diff () {
@@ -38,7 +38,7 @@ check_diff () { test_expect_success ' diff applies' ' test_when_finished "git reset --hard" && git checkout --detach "$name^" && - git apply "$name.diff" && + git apply --index "$name.diff" && git diff --exit-code "$name" ' }
Re: [PATCH v2.5 1/8] t4051: rewrite, add more tests
In reply to René ScharfeThanks.
PATCH v2 2/8 xdiff: factor out match_func_rec()
In reply to René ScharfeAdd match_func_rec(), a helper that wraps accessing a record and calling the appropriate function for checking if it contains a function line.
--- xdiff/xemit.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
xdiff/xemit.c +11 -4
@@ -120,6 +120,16 @@ static long def_ff(const char *rec, long len, char *buf, long sz, void *priv)return -1 ; } +static long match_func_rec (xdfile_t *xdf,xdemitconf_t const *xecfg,long ri, +char *buf,long sz) +{ +const char *rec; +long len =xdl_get_rec (xdf, ri, &rec); +if (!xecfg->find_func) +return def_ff (rec, len, buf, sz, xecfg->find_func_priv); +return xecfg->find_func (rec, len, buf, sz, xecfg->find_func_priv); +} +struct func_line {long len;char buf[80 ];
@@ -128,7 +138,6 @@ struct func_line {static long get_func_line (xdfenv_t *xe,xdemitconf_t const *xecfg,struct func_line *func_line,long start,long limit) { -find_func_t ff = xecfg->find_func ? xecfg->find_func : def_ff;long l, size, step = (start > limit) ?-1 :1 ;char *buf, dummy[1 ];
@@ -136,9 +145,7 @@ static long get_func_line(xdfenv_t *xe, xdemitconf_t const *xecfg, size = func_line ?sizeof (func_line->buf) :sizeof (dummy);for (l = start; l != limit &&0 <= l && l < xe->xdf1.nrec; l += step) { -const char *rec; -long reclen =xdl_get_rec (&xe->xdf1, l, &rec); -long len =ff (rec, reclen, buf, size, xecfg->find_func_priv); +long len =match_func_rec (&xe->xdf1, xecfg, l, buf, size);if (len >=0 ) {if (func_line) func_line->len = len;
Signature · 1 line
2.8.3
PATCH v2 3/8 xdiff: handle appended chunks better with -W
In reply to René ScharfeIf lines are added at the end of a file, diff -W shows the whole file. That's because get_func_line() only considers the pre-image and gives up if it sees a record index beyond its end.
Consider the post-image as well to see if the added lines already make up a full function. If it doesn't then search for the previous function line by starting from the bottom of the pre-image, thereby avoiding to confuse get_func_line().
Reuse the existing label called "again", as it's exactly where we need to jump to when we're done handling the pre-context, but rename it to "post_context_calculation" in order to document its new purpose better.
--- t/t4051-diff-function-context.sh | 2 +- xdiff/xemit.c | 27 ++++++++++++++++++++++++--- 2 files changed, 25 insertions(+), 4 deletions(-)
t/t4051-diff-function-context.sh +1 -1
@@ -131,7 +131,7 @@ test_expect_success ' context includes end' ' grep "^[+].*End of second part" extended.diff ' -test_expect_failure ' context does not include other functions' ' +test_expect_success ' context does not include other functions' ' test $(grep -c "^[ +-].*Begin" extended.diff) -le 2 '
xdiff/xemit.c +24 -3
@@ -171,7 +171,28 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb, s2 =XDL_MAX (xch->i2 - xecfg->ctxlen,0 );if (xecfg->flags &XDL_EMIT_FUNCCONTEXT ) { -long fs1 =get_func_line (xe, xecfg,NULL , xch->i1,-1 ); +long fs1, i1 = xch->i1; + +/* Appended chunk? */ +if (i1 >= xe->xdf1.nrec) { +char dummy[1 ]; + +/* +* We don't need additional context if +* a whole function was added. +*/ +if (match_func_rec (&xe->xdf2, xecfg, xch->i2, + dummy,sizeof (dummy)) >=0 ) + goto post_context_calculation; + +/* +* Otherwise get more context from the +* pre-image. +*/ + i1 = xe->xdf1.nrec -1 ; + } + + fs1 =get_func_line (xe, xecfg,NULL , i1,-1 );if (fs1 <0 ) fs1 =0 ;if (fs1 < s1) {
@@ -180,7 +201,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb, } } - again: + post_context_calculation: lctx = xecfg->ctxlen; lctx =XDL_MIN (lctx, xe->xdf1.nrec - (xche->i1 + xche->chg1)); lctx =XDL_MIN (lctx, xe->xdf2.nrec - (xche->i2 + xche->chg2));
@@ -209,7 +230,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,if (l <= e1 ||get_func_line (xe, xecfg,NULL , l, e1) <0 ) { xche = xche->next; - goto again; + goto post_context_calculation; } } }
Signature · 1 line
2.8.3
PATCH v2 4/8 xdiff: ignore empty lines before added functions with -W
In reply to René ScharfeIf a new function and a preceding empty line is appended, diff -W shows the previous function in full in order to provide context for that empty line. In most languages empty lines between sections are not interesting in and off themselves and showing a whole extra function for them is not what we want.
Skip empty lines when checking of the appended chunk starts with a function line, thereby avoiding to extend the context just for them.
--- t/t4051-diff-function-context.sh | 2 +- xdiff/xemit.c | 22 ++++++++++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-)
t/t4051-diff-function-context.sh +1 -1
@@ -117,7 +117,7 @@ test_expect_success ' context includes end' ' grep "^[+].*End of first part" appended.diff ' -test_expect_failure ' context does not include other functions' ' +test_expect_success ' context does not include other functions' ' test $(grep -c "^[ +-].*Begin" appended.diff) -le 1 '
xdiff/xemit.c +20 -2
@@ -155,6 +155,18 @@ static long get_func_line(xdfenv_t *xe, xdemitconf_t const *xecfg,return -1 ; } +static int is_empty_rec (xdfile_t *xdf,long ri) +{ +const char *rec; +long len =xdl_get_rec (xdf, ri, &rec); + +while (len >0 &&XDL_ISSPACE (*rec)) { + rec++; + len--; + } +return !len; +} +int xdl_emit_diff (xdfenv_t *xe,xdchange_t *xscr,xdemitcb_t *ecb,xdemitconf_t const *xecfg) {long s1, s2, e1, e2, lctx;
@@ -176,12 +188,18 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,/* Appended chunk? */ if (i1 >= xe->xdf1.nrec) {char dummy[1 ]; +long i2 = xch->i2;/* * We don't need additional context if -* a whole function was added. +* a whole function was added, possibly +* starting with empty lines. */ -if (match_func_rec (&xe->xdf2, xecfg, xch->i2, +while (i2 < xe->xdf2.nrec && +is_empty_rec (&xe->xdf2, i2)) + i2++; +if (i2 < xe->xdf2.nrec && +match_func_rec (&xe->xdf2, xecfg, i2, dummy,sizeof (dummy)) >=0 ) goto post_context_calculation;
Signature · 1 line
2.8.3
PATCH v2 5/8 xdiff: -W: don't include common trailing empty lines in context
In reply to René ScharfeEmpty lines between functions are shown by diff -W, as it considers them to be part of the function preceding them. They are not interesting in most languages. The previous patch stopped showing them in the special case of a function added at the end of a file.
Stop extending context to those empty lines by skipping back over them from the start of the next function.
--- t/t4051-diff-function-context.sh | 4 ++-- xdiff/xemit.c | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-)
t/t4051-diff-function-context.sh +2 -2
@@ -85,7 +85,7 @@ test_expect_success ' context does not include preceding empty lines' ' test "$(first_context_line <changed_hello.diff)" != " " ' -test_expect_failure ' context does not include trailing empty lines' ' +test_expect_success ' context does not include trailing empty lines' ' test "$(last_context_line <changed_hello.diff)" != " " '
@@ -103,7 +103,7 @@ test_expect_success ' context does not include other functions' ' test $(grep -c "^[ +-].*Begin" changed_includes.diff) -le 1 ' -test_expect_failure ' context does not include trailing empty lines' ' +test_expect_success ' context does not include trailing empty lines' ' test "$(last_context_line <changed_includes.diff)" != " " '
xdiff/xemit.c +2 -0
@@ -231,6 +231,8 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,long fe1 =get_func_line (xe, xecfg,NULL , xche->i1 + xche->chg1, xe->xdf1.nrec); +while (fe1 >0 &&is_empty_rec (&xe->xdf1, fe1 -1 )) + fe1--;if (fe1 <0 ) fe1 = xe->xdf1.nrec;if (fe1 > e1) {
Signature · 1 line
2.8.3
PATCH v2 6/8 xdiff: don't trim common tail with -W
In reply to René ScharfeThe function trim_common_tail() exits early if context lines are requested. If -U0 and -W are specified together then it can still trim context lines that might belong to a changed function. As a result that function is shown incompletely.
Fix that by calling trim_common_tail() only if no function context or fixed context is requested. The parameter ctx is no longer needed now; remove it.
While at it fix an outdated comment as well.
--- t/t4051-diff-function-context.sh | 2 +- xdiff-interface.c | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-)
t/t4051-diff-function-context.sh +1 -1
@@ -145,7 +145,7 @@ test_expect_success ' context includes begin' ' grep "^ .*Begin of first part" long_common_tail.diff ' -test_expect_failure ' context includes end' ' +test_expect_success ' context includes end' ' grep "^ .*End of second part" long_common_tail.diff '
xdiff-interface.c +4 -6
@@ -100,9 +100,9 @@ static int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)/* * Trim down common substring at the end of the buffers, -* but leave at least ctx lines at the end. +* but end on a complete line. */ -static void trim_common_tail (mmfile_t *a,mmfile_t *b,long ctx) +static void trim_common_tail (mmfile_t *a,mmfile_t *b) {const int blk =1024 ;long trimmed =0 , recovered =0 ;
@@ -110,9 +110,6 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b, long ctx)char *bp = b->ptr + b->size;long smaller = (a->size < b->size) ? a->size : b->size; -if (ctx) -return ; -while (blk + trimmed <= smaller && !memcmp (ap - blk, bp - blk, blk)) { trimmed += blk; ap -= blk;
@@ -134,7 +131,8 @@ int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t coif (mf1->size >MAX_XDIFF_SIZE || mf2->size >MAX_XDIFF_SIZE )return -1 ; -trim_common_tail (&a, &b, xecfg->ctxlen); +if (!xecfg->ctxlen && !(xecfg->flags &XDL_EMIT_FUNCCONTEXT )) +trim_common_tail (&a, &b);return xdl_diff (&a, &b, xpp, xecfg, xecb); }
Signature · 1 line
2.8.3
PATCH v2 7/8 t7810: add test for grep -W and trailing empty context lines
In reply to René ScharfeAdd a test demonstrating that git grep -W prints empty lines following the function context we're actually interested in. The modified test file makes it necessary to adjust three unrelated test cases.
--- t/t7810-grep.sh | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)
t/t7810-grep.sh +16 -3
@@ -9,7 +9,9 @@ test_description='git grep various. . ./test-lib.sh cat >hello.c <<EOF +#include <assert.h> #include <stdio.h> + int main(int argc, const char **argv) { printf("Hello world.\n");
@@ -715,6 +717,7 @@ test_expect_success 'grep -p' ' cat >expected <<EOF hello.c-#include <stdio.h> +hello.c- hello.c=int main(int argc, const char **argv) hello.c-{ hello.c- printf("Hello world.\n");
@@ -741,6 +744,16 @@ test_expect_success 'grep -W' ' ' cat >expected <<EOF +hello.c-#include <assert.h> +hello.c:#include <stdio.h> +EOF + +test_expect_failure 'grep -W shows no trailing empty lines' ' + git grep -W stdio >actual && + test_cmp expected actual +' + +cat >expected <<EOF hello.c= printf("Hello world.\n"); hello.c: return 0; hello.c- /* char ?? */
@@ -1232,8 +1245,8 @@ test_expect_success 'grep --heading' ' cat >expected <<EOF <BOLD;GREEN>hello.c<RESET> -2:int main(int argc, const <BLACK;BYELLOW>char<RESET> **argv) -6: /* <BLACK;BYELLOW>char<RESET> ?? */ +4:int main(int argc, const <BLACK;BYELLOW>char<RESET> **argv) +8: /* <BLACK;BYELLOW>char<RESET> ?? */ <BOLD;GREEN>hello_world<RESET> 3:Hel<BLACK;BYELLOW>lo_w<RESET>orld
@@ -1340,7 +1353,7 @@ test_expect_success 'grep --color -e A --and --not -e B with context' ' ' cat >expected <<EOF -hello.c-#include <stdio.h> +hello.c- hello.c=int main(int argc, const char **argv) hello.c-{ hello.c: pr<RED>int<RESET>f("<RED>Hello<RESET> world.\n");
Signature · 1 line
2.8.3
PATCH v2 8/8 grep: -W: don't extend context to trailing empty lines
In reply to René ScharfeEmpty lines between functions are shown by grep -W, as it considers them to be part of the function preceding them. They are not interesting in most languages. The previous patches stopped showing them for diff -W.
Stop showing empty lines trailing a function with grep -W. Grep scans the lines of a buffer from top to bottom and prints matching lines immediately. Thus we need to peek ahead in order to determine if an empty line is part of a function body and worth showing or not.
Remember how far ahead we peeked in order to avoid having to do so repeatedly when handling multiple consecutive empty lines.
--- grep.c | 28 ++++++++++++++++++++++++++-- t/t7810-grep.sh | 2 +- 2 files changed, 27 insertions(+), 3 deletions(-)
grep.c +26 -2
@@ -1396,9 +1396,17 @@ static int fill_textconv_grep(struct userdiff_driver *driver,return 0 ; } +static int is_empty_line (const char *bol,const char *eol) +{ +while (bol < eol &&isspace (*bol)) + bol++; +return bol == eol; +} +static int grep_source_1 (struct grep_opt *opt,struct grep_source *gs,int collect_hits) {char *bol; +char *peek_bol =NULL ;unsigned long left;unsigned lno =1 ;unsigned last_hit =0 ;
@@ -1543,8 +1551,24 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle show_function =1 ; goto next_line; } -if (show_function &&match_funcname (opt, gs, bol, eol)) - show_function =0 ; +if (show_function && (!peek_bol || peek_bol < bol)) { +unsigned long peek_left = left; +char *peek_eol = eol; + +/* +* Trailing empty lines are not interesting. +* Peek past them to see if they belong to the +* body of the current function. +*/ + peek_bol = bol; +while (is_empty_line (peek_bol, peek_eol)) { + peek_bol = peek_eol +1 ; + peek_eol =end_of_line (peek_bol, &peek_left); + } + +if (match_funcname (opt, gs, peek_bol, peek_eol)) + show_function =0 ; + }if (show_function || (last_hit && lno <= last_hit + opt->post_context)) {/* If the last hit is within the post context,
t/t7810-grep.sh +1 -1
@@ -748,7 +748,7 @@ hello.c-#include <assert.h> hello.c:#include <stdio.h> EOF -test_expect_failure 'grep -W shows no trailing empty lines' ' +test_expect_success 'grep -W shows no trailing empty lines' ' git grep -W stdio >actual && test_cmp expected actual '
Signature · 1 line
2.8.3