Re: [PATCH v3] blame: prevent error if range ends past end of file

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Oct 27, 2017 at 2:18 AM, Isabella Stephens
<istephens@atlassian.com> wrote:
> On 27/10/17 12:58 pm, Junio C Hamano wrote:
>> There should be an "is the range sensible?" check after all the
>> tweaking to bottom and top are done, I think.
>
> My mistake. I missed that case. I think this section of code is a little
> hard to read because it avoids treating an empty file as a special case.
> Why not do something like this:
>
>   for (range_i = 0; range_i < range_list.nr; ++range_i) {
>           long bottom, top;
>           if (!lno)
>                   die(_("file is empty"));

No need for this conditional to reside within the loop. Hoisting it
outside the loop would (IMO) make the intent even clearer:

    if (range_list.nr && !lno)
        die(_("file is empty; -L has no effect"));
    for (...) {
        ...

On the other hand, one could argue that "-L," (where comma is the sole
argument to -L), which specifies the entire file, should be allowed
even on an empty file without complaining that the file is empty.
Although it may not seem sensible for a human to specify "-L," perhaps
a tool would do so to override an earlier more restrictive -L range.
However, that may be too esoteric a case to worry about.

>           if (parse_range_arg(range_list.items[range_i].string,
>                               nth_line_cb, &sb, lno, anchor,
>                               &bottom, &top, sb.path))
>                   usage(blame_usage);
>           if (bottom < 1)
>                   bottom = 1;
>           if (lno < top)
>                   top = lno;
>           if (top < 0 || lno < bottom)
>                    die(Q_("file %s has only %lu line",
>                          "file %s has only %lu lines",
>                          lno), path, lno);
>           bottom--;
>           range_set_append_unsafe(&ranges, bottom, top);
>           anchor = top + 1;