[Bug?] log -p -W showing the whole file for a patch that adds to the end?

[Bug?] log -p -W showing the whole file for a patch that adds to the end?

[Junio C Hamano]

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).

Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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).

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 xdiff/xemit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 993724b..4e58482 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -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;

Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?

[René Scharfe]

Am 11.05.2016 um 00:51 schrieb Junio C Hamano:
> 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é

Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?

[René Scharfe]

Am 11.05.2016 um 00:51 schrieb Junio C Hamano:
> 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()

[René Scharfe]

Add match_func_rec(), a helper that wraps accessing a record and calling
the appropriate function for checking if it contains a function line.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
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(-)

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 993724b..0c87637 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -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;
-- 
2.8.3

[PATCH 2/5] diff: handle appended chunks better with -W

[René Scharfe]

If 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.

Reported-by: Junio C Hamano <gitster@pobox.com>
Initial-patch-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 xdiff/xemit.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 0c87637..969100d 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -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;
 				}
 			}
 		}
-- 
2.8.3

[PATCH 3/5] diff: ignore empty lines before added functions with -W

[René Scharfe]

If 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.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 xdiff/xemit.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 969100d..087956a 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -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;
 
-- 
2.8.3

[PATCH 4/5] diff: don't include common trailing empty lines with -W

[René Scharfe]

Empty 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.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 xdiff/xemit.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 087956a..d0c0738 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -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) {
-- 
2.8.3

[PATCH 5/5] grep: don't extend context to trailing empty lines with -W

[René Scharfe]

Empty 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.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 grep.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/grep.c b/grep.c
index ec6f7ff..1e15b62 100644
--- a/grep.c
+++ b/grep.c
@@ -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,
-- 
2.8.3

Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

>   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(-)

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?

Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?

[René Scharfe]

Am 24.05.2016 um 20:16 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>>    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(-)

> 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é

Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 24.05.2016 um 20:16 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>>    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(-)
>
>> 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.

Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?

[René Scharfe]

Am 26.05.2016 um 19:05 schrieb Junio C Hamano:
> 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é

Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?

[René Scharfe]

Am 21.05.2016 um 20:42 schrieb René Scharfe:
> Am 11.05.2016 um 00:51 schrieb Junio C Hamano:
>> 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(-)

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


diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index d0c0738..bfa53d3 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -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

[René Scharfe]

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.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 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

diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index 001d678..a3adba4 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -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
diff --git a/t/t4051/appended1.c b/t/t4051/appended1.c
new file mode 100644
index 0000000..a9f56f1
--- /dev/null
+++ b/t/t4051/appended1.c
@@ -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");
diff --git a/t/t4051/appended2.c b/t/t4051/appended2.c
new file mode 100644
index 0000000..e651f71
--- /dev/null
+++ b/t/t4051/appended2.c
@@ -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
diff --git a/t/t4051/dummy.c b/t/t4051/dummy.c
new file mode 100644
index 0000000..a43016e
--- /dev/null
+++ b/t/t4051/dummy.c
@@ -0,0 +1,7 @@
+
+static int dummy(void)	// Begin of dummy
+{
+	int rc = 0;
+
+	return rc;
+}	// End of dummy
diff --git a/t/t4051/hello.c b/t/t4051/hello.c
new file mode 100644
index 0000000..63b1a1e
--- /dev/null
+++ b/t/t4051/hello.c
@@ -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
diff --git a/t/t4051/includes.c b/t/t4051/includes.c
new file mode 100644
index 0000000..efc68f8
--- /dev/null
+++ b/t/t4051/includes.c
@@ -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>
-- 
2.8.3

[PATCH v2 2/8] xdiff: factor out match_func_rec()

[René Scharfe]

Add match_func_rec(), a helper that wraps accessing a record and calling
the appropriate function for checking if it contains a function line.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 xdiff/xemit.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 993724b..0c87637 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -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;
-- 
2.8.3

[PATCH v2 3/8] xdiff: handle appended chunks better with -W

[René Scharfe]

If 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.

Reported-by: Junio C Hamano <gitster@pobox.com>
Initial-patch-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 t/t4051-diff-function-context.sh |  2 +-
 xdiff/xemit.c                    | 27 ++++++++++++++++++++++++---
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index a3adba4..d78461d 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -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
 '
 
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 0c87637..969100d 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -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;
 				}
 			}
 		}
-- 
2.8.3

[PATCH v2 4/8] xdiff: ignore empty lines before added functions with -W

[René Scharfe]

If 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.

Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 t/t4051-diff-function-context.sh |  2 +-
 xdiff/xemit.c                    | 22 ++++++++++++++++++++--
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index d78461d..b191c655 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -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
 '
 
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 969100d..29cec12 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -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;
 
-- 
2.8.3

[PATCH v2 5/8] xdiff: -W: don't include common trailing empty lines in context

[René Scharfe]

Empty 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.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 t/t4051-diff-function-context.sh | 4 ++--
 xdiff/xemit.c                    | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index b191c655..9fe590f 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -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)" != " "
 '
 
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 29cec12..bfa53d3 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -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) {
-- 
2.8.3

[PATCH v2 6/8] xdiff: don't trim common tail with -W

[René Scharfe]

The 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.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 t/t4051-diff-function-context.sh |  2 +-
 xdiff-interface.c                | 10 ++++------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index 9fe590f..88a3308 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -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
 '
 
diff --git a/xdiff-interface.c b/xdiff-interface.c
index 54236f2..f34ea76 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -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 co
 	if (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);
 }
-- 
2.8.3

[PATCH v2 7/8] t7810: add test for grep -W and trailing empty context lines

[René Scharfe]

Add 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.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 t/t7810-grep.sh | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 1e72971..26912dc 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -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");
-- 
2.8.3

[PATCH v2 8/8] grep: -W: don't extend context to trailing empty lines

[René Scharfe]

Empty 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.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 grep.c          | 28 ++++++++++++++++++++++++++--
 t/t7810-grep.sh |  2 +-
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/grep.c b/grep.c
index ec6f7ff..1e15b62 100644
--- a/grep.c
+++ b/grep.c
@@ -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,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 26912dc..960425a 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -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
 '
-- 
2.8.3

Re: [PATCH v2 1/8] t4051: rewrite, add more tests

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> +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).

> +	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.

> +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.

> +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.

> +		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

[René Scharfe]

Am 30.05.2016 um 01:55 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> +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.

>> +	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. ;)

>> +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).

>> +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.

Right.  It's just there to make that intent clear.

>> +		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 v2.5 1/8] t4051: rewrite, add more tests

[René Scharfe]

Remove 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.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
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

diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index 001d678..ca01725 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -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
diff --git a/t/t4051/appended1.c b/t/t4051/appended1.c
new file mode 100644
index 0000000..a9f56f1
--- /dev/null
+++ b/t/t4051/appended1.c
@@ -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");
diff --git a/t/t4051/appended2.c b/t/t4051/appended2.c
new file mode 100644
index 0000000..e651f71
--- /dev/null
+++ b/t/t4051/appended2.c
@@ -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
diff --git a/t/t4051/dummy.c b/t/t4051/dummy.c
new file mode 100644
index 0000000..a43016e
--- /dev/null
+++ b/t/t4051/dummy.c
@@ -0,0 +1,7 @@
+
+static int dummy(void)	// Begin of dummy
+{
+	int rc = 0;
+
+	return rc;
+}	// End of dummy
diff --git a/t/t4051/hello.c b/t/t4051/hello.c
new file mode 100644
index 0000000..63b1a1e
--- /dev/null
+++ b/t/t4051/hello.c
@@ -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
diff --git a/t/t4051/includes.c b/t/t4051/includes.c
new file mode 100644
index 0000000..efc68f8
--- /dev/null
+++ b/t/t4051/includes.c
@@ -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>
-- 
2.8.3

Re: [PATCH v2.5 1/8] t4051: rewrite, add more tests

[René Scharfe]

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(-)

diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index 88a3308..b6bb04a 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -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

[Junio C Hamano]

Thanks.