On Fri, Oct 27, 2017 at 2:18 AM, Isabella Stephens
<[email protected]> 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;
