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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 27, 2017, 06:56 UTC
Message-ID: <CAPig+cSH-y7ChDE_LW55M_wB=KhD7uJHXX=OvZ=1ri5KfYQ5Sg@mail.gmail.com>
In-Reply-To: <926900b8-1aad-b02a-b3c6-2bc4930233fd@atlassian.com>

On Fri, Oct 27, 2017 at 2:18 AM, Isabella Stephens <istephens@atlassian.com> wrote:

Show 12 quoted lines

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

Show 15 quoted lines

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