On Fri, Oct 27, 2017 at 2:18 AM, Isabella Stephens
[off-list ref] wrote:

On 27/10/17 12:58 pm, Junio C Hamano wrote:

quoted

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;