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

From: René Scharfe <l.s.r@web.de>
Date: May 28, 2016, 14:46 UTC
Message-ID: <5749AF59.2070704@web.de>
In-Reply-To: <5740AC28.6010202@web.de>

Am 21.05.2016 um 20:42 schrieb René Scharfe:

Show 44 quoted lines

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