Fix plan build regression in 0.76.0 release

[baumanj]

tl;dr see the fix in 05d3623

Full Details

Reverts #6248
Resolves #6250

Now that we understand the nature of this error, we can re-implement that shellcheck fixes that we previously reverted and add the necessary fix to the problem they uncovered.

The Bugs

1. In _resolve_scaffolding_dependencies (and other similar usages), changing the code that collects the transitive dependencies from

   sdeps=($(_get_deps_for "$resolved"))

   to

   read -r -a sdeps <<< "$(_get_deps_for "$resolved")"

   as part of Enable clippy and shellcheck linting  #6160 to address shellcheck lint SC2207. This resulted in a defect where because only the first dependency from _get_deps_for was added to sdeps, scaffolding-based builds failed. [plan-build] Avoid reading a newline delimited value to array #6240 was submitted to fix the issue.

2. The fix in [plan-build] Avoid reading a newline delimited value to array #6240 was correct. It changed

   read -r -a sdeps <<< "$(_get_deps_for "$resolved")"

   to

   mapfile -t sdeps < <(_get_deps_for "$resolved")

   The first command is for populating an array from a single line of input, with entries separated by whitespace. The second command is for populating an array from multiple lines of input, with entries separate by newlines (which is what _get_deps_for does).

   However, a different bug was discovered: unexpected newlines were appearing at the beginning of the TDEPS files for some packages, causing breakages. Eventually, the difference in behavior between

   sdeps=($(_get_deps_for "$resolved"))

   and

   mapfile -t sdeps < <(_get_deps_for "$resolved")

   was traced back to _get_deps_for:

   # **Internal** Returns (on stdout) the `TDEPS` file contents of another locally
   # installed package which contain the set of all direct and transitive run
   # dependencies. An empty set could be returned as whitespace and/or newlines.
   # The lack of a `TDEPS` file in the desired package will be considered an
   # unset, or empty set.
   #
   # ```
   # _get_tdeps_for /hab/pkgs/acme/a/4.2.2/20160113044458
   # # /hab/pkgs/acme/dep-b/1.2.3/20160113033619
   # # /hab/pkgs/acme/dep-c/5.0.1/20160113033507
   # # /hab/pkgs/acme/dep-d/2.0.0/20160113033539
   # # /hab/pkgs/acme/dep-e/10.0.1/20160113033453
   # # /hab/pkgs/acme/dep-f/4.2.2/20160113033338
   # # /hab/pkgs/acme/dep-g/4.2.2/20160113033319
   # ```
   #
   # Will return 0 in any case and the contents of `TDEPS` if the file exists.
   _get_tdeps_for() {
     local pkg_path="$1"
     if [[ -f "$pkg_path/TDEPS" ]]; then
       cat "$pkg_path"/TDEPS
     else
       # No file, meaning an empty set
       echo
     fi
     return 0
   }

   Because _get_tdeps_for returned a newline (rather than nothing) in the case of an absent TDEPS file (which is normal, some packages have no transitive dependencies), the mapfile version of the code interpreted this as a single element array, containing an empty string. This was eventually rendered into the subsequent TDEPS file as an empty line.

   Essentially, the previous code depended upon "the shell's sloppy word splitting and glob expansion" that shellcheck warns against. Updating _get_tdeps_for (and similarly _get_deps_for) to output nothing in the case of an empty set (and omitting the superfluous return):

   _get_tdeps_for() {
     local pkg_path="$1"
     if [[ -f "$pkg_path/TDEPS" ]]; then
       cat "$pkg_path"/TDEPS
     fi
   }

   fixes the issue.

This PR makes some additional improvements to _get_[t]deps_for; see 05d3623.

[thesentinels]

Thanks for the pull request! Here is what will happen next:

1. Your PR will be reviewed by the maintainers
2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

[christophermaier]

Looks good to me, though I'll defer ultimately to @smacfarlane @eeyun and @fnichol for their validation.

Nice work @baumanj

[raskchanky]

Nice work

[raskchanky]

[eeyun]

Discussion on whether to merge this now or wait until we have some scripted tests is taking place in slack. As a note on the issue, the cases we were testing on Friday that were failing appear to be fine in this PR branch but as @smacfarlane pointed out there's still some concern around moving forward with reverting the reversion without some tests that can tell us that there aren't any other unforseen ripples from the change.

[eeyun]

The decision here is to merge this, thaw the merge-freeze and backfill tests while this is in acceptance before the next release! Great work on these fixes @baumanj.

[smacfarlane]

I've validated this change fixes the issues we ran into last week, and the BUILD_TDEPS and TDEPS files contain the correct entries without newlines.

I have one question around syntax which I've left inline, but that's not a blocker to merging.

[smacfarlane]

@baumanj Could you provide a brief comment inline describing what the < <() syntax means? I'm having a little trouble understanding it. I see in your pr you link to the shellcheck page that describes that this is the preferred style, but didn't see an explanation of the syntax. Is it a particular bash idiom we could link to?

[baumanj]

Added d60e521