How Should `batslib_count_lines` Handle Trailing Newlines?

Currently, batslib_count_lines is written to try and ignore the last newline to be equivalent to it not being there.

This leads '' and '\n' as both being "0 line". Similarly, 'a' and 'a\n' will both be considered as "1 line". This is explicitly checked in tests.

Issue

However, when using run --keep-empty-lines, assertions will check trailing newlines. However, this leads batslib_is_single_line to still return 0, and the error messages will be printed by batslib_print_kv_single instead of batslib_print_kv_multi (which shows the line numbers, a helpful indicator for debugging). Example (using bats-assert):

@test "trailing empty line" {
  run --keep-empty-lines printf 'a\n'
  assert_output "$(printf 'a')"
}

prints:

-- output differs --
expected : a
actual   : a

--

The new line is there, but it's hard to really notice. More egregiously, a multi-line output will be very misleading. Example:

@test "trailing empty line on multiline" {
  run --keep-empty-lines printf 'a\nb\n'
  assert_output "$(printf 'a\nb')"
}

prints:

-- output differs --
expected (2 lines):
a
b
actual (2 lines):
a
b
--

This both hides the extra new line and states that the line counts are the same.

Discussion

I'm writing out this GH Issue (and not just sending some PR(s)) because there's a few ways that this could possibly be addressed. I wanted to chat this over and see which way you'd think would be best. I'm happy to put together a PR for wherever we land.

Here's some options that I've considered, feel free to suggest more:

1. Update the impl for `batslib_count_lines`.

This could be done by changing

batslib_count_lines() {
  local -i n_lines=0
  local line
  while IFS='' read -r line || [[ -n $line ]]; do
    (( ++n_lines ))
  done < <(printf '%s' "$1")
  echo "$n_lines"
}

batslib_count_lines() {
  local -r input="$1"
  local -i n_lines=0
  local line
  if [ -n "$input" ]; then
    while IFS='' read -r line; do
      (( ++n_lines ))
    done < <(printf '%s' "$input")
    # Increment for the EOF (not captured by the loop above).
    (( ++n_lines ))
  fi
  echo "$n_lines"
}

There are some related extra changes that are necessary with this approach. batslib_prefix needs to be updated in a similar way (else batslib_print_kv_multi in batslib_print_kv_single_or_multi will print the incorrect number of lines). Potential impl change from

batslib_prefix() {
  local -r prefix="${1:-  }"
  local line
  while IFS='' read -r line || [[ -n $line ]]; do
    printf '%s%s\n' "$prefix" "$line"
  done
}

batslib_prefix() {
  local -r prefix="${1:-  }"
  local line=''
  local -i lines_read=0
  while IFS='' read -r line; do
    printf '%s%s\n' "$prefix" "$line"
    (( ++lines_read ))
  done
  # Print for the line with EOF (not captured by the loop above).
  # Do not print if stdin was completely empty.
  if (( lines_read > 0 )) || [[ -n "$line" ]]; then
    printf '%s%s\n' "$prefix" "$line"
  fi
}

batslib_mark would likely also need to be updated in a similar way (these 3 functions all use while IFS='' read -r line || [[ -n $line ]]; do).

While I think this is pretty straightforward (and my personal preference), I understand that this may have backwards compatibility issues and may have implications for other functionality built upon this function (which may really care about ignoring trailing newlines).

I can see that bats-assert makes use of these 3 functions in refute_line. The suggested changes to batslib_prefix and batslib_mark wouldn't really change anything. Though the indirect change to batslib_is_single_line (via the change to batslib_count_lines) might lead to some more calls being made to batslib_print_kv_multi instead of batslib_print_kv_single. I'd suspect that wouldn't affect most cases, as the trailing newline is removed unless explicitly forced to be kept (which is likely desirable).

2. Add `--keep-empty-lines` (or similar) to `batslib_count_lines`.

This option keeps the old functionality and allows the caller to change into tracking all newlines. This could either be done as opt-in with --keep-empty-lines or --include-trailing-newline; or it could be opt-out, where the default is to include it and --ignore-trailing-newline is added.

This option (specifically the opt-in version) is more backwards compatible with existing logic. However, it does require adding/piping the flag through various functions, which can be a bit verbose. Concerns about differentiating --keep... from intended input can be worked-around via there should always be an even number of positional arguments to these specific functions.

3. Create alternative `batslib_count_lines` function.

In this option, a new function (e.g. batslib_count_all_lines) can be added to use the new logic. This is very backwards compatible, as it does not touch the existing batslib_count_lines function. However, it would require doing the same for other functions like batslib_is_single_line, batslib_print_kv_multi, batslib_print_kv_single_or_multi, etc.

Those are the suggestions that I have thus far. As I mentioned, I think option 1 is the best option. Let me know what you think.

Thanks!

P.S. There's a psudo-related TODO(ztombol) on batslib_count_lines about fixing inconsistencies vs ${#lines[@]}. This isn't really the same issue, but it's somewhat related. Thought I'd mention it.

bats-core / bats-support