Re: [PATCH v4 1/2] blame: prevent error if range ends past end of file

Isabella Stephens [off-list ref] writes:

This is the existing behaviour. -L10,-20 for example will blame the
first 10 lines of a file, it will not fail. My patch doesn't change
this. The case I am discussing is -L,-20 which at the moment blames
the first line of the file. Trying to go backwards from the start of
a file should be considered invalid, in my opinion, however I don't
feel strongly about it - I don't expect this case is common in 
practice.

I tend to think that -L,-20 is a sloppy spelling of -L1,-20
(i.e. anything omitted gets reasonable default, and for something
that specifies both ends, i.e. "<begin>,<end>", the beginning and
the end of the file would be such reasonable default, respectively),
and as such I would imagine that the user would expect the same
behaviour as -L1,-20.  If the longhand version gives only the first
line (i.e. show up to 20 lines ending at line #1), I'd say that
sounds sensible.

Or does -L1,-20 show nothing and consider the input invalid?  If so,
then sure, -L,-20 should also be an invalid input.

Okay, I think that sounds reasonable. I've amended the patch so that the
line number is clipped to 1 when reading in a line range. And -L,-n is now
treated the same as -L1,-n i.e. it will blame (or log) the first line of the
file only.

From: Isabella Stephens <redacted>

If the -L option is used to specify a line range in git blame, and the
end of the range is past the end of the file, git will fail with a fatal
error. This commit prevents such behavior - instead we display the blame
for existing lines within the specified range. Tests and documentation
are ammended accordingly.

This commit also fixes two corner cases. Blaming -L n,-(n+1) now blames
the first n lines of a file rather than from n to the end of the file.
Blaming -L ,-n will be treated as -L 1,-n and blame the first line of
the file, rather than blaming the whole file.

Signed-off-by: Isabella Stephens <redacted>
---
 Documentation/git-blame.txt   | 10 ++++++++++
 builtin/blame.c               |  4 ++--
 line-range.c                  |  2 +-
 t/t8003-blame-corner-cases.sh | 12 ++++++++----
 4 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
index 16323eb80..8cb81f57a 100644
--- a/Documentation/git-blame.txt
+++ b/Documentation/git-blame.txt

@@ -152,6 +152,16 @@ Also you can use a regular expression to specify the line range:
 
 which limits the annotation to the body of the `hello` subroutine.
 
+A range that begins or ends outside the bounds of the file will
+blame the relevant lines. For example:
+
+	git blame -L 10,-20 foo
+	git blame -L 10,+20 foo
+
+will respectively blame the first 10 and last 11 lines of a
+20 line file. However, blaming a line range that is entirely
+outside the bounds of the file will fail.
+
 When you are not interested in changes older than version
 v2.6.18, or changes older than 3 weeks, you can use revision
 range specifiers  similar to 'git rev-list':

diff --git a/builtin/blame.c b/builtin/blame.c
index 9dcb367b9..e1359b192 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c

@@ -886,13 +886,13 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 				    nth_line_cb, &sb, lno, anchor,
 				    &bottom, &top, sb.path))
 			usage(blame_usage);
-		if (lno < top || ((lno || bottom) && lno < bottom))
+		if ((!lno && (top || bottom)) || lno < bottom)
 			die(Q_("file %s has only %lu line",
 			       "file %s has only %lu lines",
 			       lno), path, lno);
 		if (bottom < 1)
 			bottom = 1;
-		if (top < 1)
+		if (top < 1 || lno < top)
 			top = lno;
 		bottom--;
 		range_set_append_unsafe(&ranges, bottom, top);

diff --git a/line-range.c b/line-range.c
index 323399d16..232c3909e 100644
--- a/line-range.c
+++ b/line-range.c

@@ -47,7 +47,7 @@ static const char *parse_loc(const char *spec, nth_line_fn_t nth_line,
 			else if (!num)
 				*ret = begin;
 			else
-				*ret = begin + num;
+				*ret = begin + num > 0 ? begin + num : 1;
 			return term;
 		}
 		return spec;

diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
index 661f9d430..c92a47b6d 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh

@@ -216,14 +216,18 @@ test_expect_success 'blame -L with invalid start' '
 '
 
 test_expect_success 'blame -L with invalid end' '
-	test_must_fail git blame -L1,5 tres 2>errors &&
-	test_i18ngrep "has only 2 lines" errors
+	git blame -L1,5 tres >out &&
+	test_line_count = 2 out
 '
 
 test_expect_success 'blame parses <end> part of -L' '
 	git blame -L1,1 tres >out &&
-	cat out &&
-	test $(wc -l < out) -eq 1
+	test_line_count = 1 out
+'
+
+test_expect_success 'blame -Ln,-(n+1)' '
+	git blame -L3,-4 nine_lines >out &&
+	test_line_count = 3 out
 '
 
 test_expect_success 'indent of line numbers, nine lines' '

-- 
2.14.3 (Apple Git-98)

From: Isabella Stephens <redacted>

If the -L option is used to specify a line range in git log, and the end
of the range is past the end of the file, git will fail with a fatal
error. This commit prevents such behaviour - instead we perform the log
for existing lines within the specified range.

This commit also fixes a corner case where -L ,-n:file would be treated
as a log over the whole file. Now we treat this as -L 1,-n:file and
blame the first line of the file instead.

Signed-off-by: Isabella Stephens <redacted>
---
 line-log.c          | 8 +++++---
 t/t4211-line-log.sh | 8 +-------
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/line-log.c b/line-log.c
index cdc2257db..4da40dec1 100644
--- a/line-log.c
+++ b/line-log.c

@@ -599,11 +599,13 @@ parse_lines(struct commit *commit, const char *prefix, struct string_list *args)
 				    lines, anchor, &begin, &end,
 				    full_name))
 			die("malformed -L argument '%s'", range_part);
-		if (lines < end || ((lines || begin) && lines < begin))
-			die("file %s has only %lu lines", name_part, lines);
+		if ((!lines && (begin || end)) || lines < begin)
+			die(Q_("file %s has only %lu line",
+				   "file %s has only %lu lines",
+				   lines), name_part, lines);
 		if (begin < 1)
 			begin = 1;
-		if (end < 1)
+		if (end < 1 || lines < end)
 			end = lines;
 		begin--;
 		line_log_data_insert(&ranges, full_name, begin, end);

diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index d0377fae5..c617347c4 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh

@@ -60,7 +60,6 @@ test_bad_opts "-L 1:nonexistent" "There is no path"
 test_bad_opts "-L 1:simple" "There is no path"
 test_bad_opts "-L '/foo:b.c'" "argument not .start,end:file"
 test_bad_opts "-L 1000:b.c" "has only.*lines"
-test_bad_opts "-L 1,1000:b.c" "has only.*lines"
 test_bad_opts "-L :b.c" "argument not .start,end:file"
 test_bad_opts "-L :foo:b.c" "no match"
 

@@ -86,12 +85,7 @@ test_expect_success '-L ,Y (Y == nlines)' '
 
 test_expect_success '-L ,Y (Y == nlines + 1)' '
 	n=$(expr $(wc -l <b.c) + 1) &&
-	test_must_fail git log -L ,$n:b.c
-'
-
-test_expect_success '-L ,Y (Y == nlines + 2)' '
-	n=$(expr $(wc -l <b.c) + 2) &&
-	test_must_fail git log -L ,$n:b.c
+	git log -L ,$n:b.c
 '
 
 test_expect_success '-L with --first-parent and a merge' '

-- 
2.14.3 (Apple Git-98)

On Tue, May 29, 2018 at 1:30 AM,  [off-list ref] wrote:

If the -L option is used to specify a line range in git blame, and the
end of the range is past the end of the file, git will fail with a fatal
error. This commit prevents such behavior - instead we display the blame
for existing lines within the specified range.

Makes sense; the new behavior is intuitive and more friendly.

Tests and documentation are ammended accordingly.

s/ammended/amended/

This documentation seems misplaced. Rather than inserting it after the
discussion of -L/regex/, a more natural place would be just above
-L/regex/ where -L<begin>,<end> is discussed.

However, I am not at all convinced that this behavior should be
documented to this level of detail. Doing so assigns too much emphasis
to what should be intuitive, thus wastes readers' time wondering why
it is so heavily emphasized. At _most_, I would think you could say
merely:

    A range that begins or ends outside the bounds of the file
    will be clipped to the file's extent.

and drop the example and discussion of the example results altogether.

In fact, because this new behavior is what most users will intuitively
expect, it might be perfectly reasonable to not say anything about it
at all (that is, don't modify git-blame.txt).

Thanks.

On Tue, May 29, 2018 at 1:30 AM,  [off-list ref] wrote:

Not sure why you removed the 'n+2' test which was added intentionally,
along with the 'n' and 'n+1' tests, to probe the boundary handling for
correctness. By eliminating 'n+2', coverage is reduced and, even
though your change might be correct at this boundary, some future
breaking change might go undetected.

On 30/5/18 6:45 pm, Eric Sunshine wrote:

quoted

diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt

@@ -152,6 +152,16 @@ Also you can use a regular expression to specify the line range:
 which limits the annotation to the body of the `hello` subroutine.

+A range that begins or ends outside the bounds of the file will
+blame the relevant lines. For example:
+
+       git blame -L 10,-20 foo
+       git blame -L 10,+20 foo
+
+will respectively blame the first 10 and last 11 lines of a
+20 line file. However, blaming a line range that is entirely
+outside the bounds of the file will fail.

This documentation seems misplaced. Rather than inserting it after the
discussion of -L/regex/, a more natural place would be just above
-L/regex/ where -L<begin>,<end> is discussed.

However, I am not at all convinced that this behavior should be
documented to this level of detail. Doing so assigns too much emphasis
to what should be intuitive, thus wastes readers' time wondering why
it is so heavily emphasized. At _most_, I would think you could say
merely:

    A range that begins or ends outside the bounds of the file
    will be clipped to the file's extent.

and drop the example and discussion of the example results altogether.

In fact, because this new behavior is what most users will intuitively
expect, it might be perfectly reasonable to not say anything about it
at all (that is, don't modify git-blame.txt)

Thanks for reviewing Eric. I've submitted a v6 patch in response to your
feedback.

I agree that given the behavior is intuitive it's not necessary to
document this change, so I've reverted the change to git-blame.txt
entirely.