0

I have some bash code that currently works correctly. However the syntax is somewhat verbose and I would appreciate any advice on how to shorten the following bash code.

For complete reference the entire script can can be found here: https://gist.github.com/sacvalleytech/951f9eb98625f983f8f4dab623f5918b

# find new deps
# $1 -> file to search for dependencies
# $2 -> array of previous dependency changes (from siblings; used to invalidate cache); code not shown
# $3 -> hierarchy of dependencies (for circular ref checking); code not shown
DEP_OUT=( "$(changes "$DEP" "${DEP_OUT[*]}" "${DEP_HIERARCHY[*]}")" )

# >>> its really annoying to do this 
# >>> to capture the return code before sorting the array
# >>> sorting the array BEFORE causes the return code to be lost
[ "$?" -eq 0 ] && DEP_CHANGED=true

# sort values | normalize paths | uniq values
DEP_OUT=( $(printf "%s\n" "${DEP_OUT[@]}" | sort | trim | uniq) )

UPDATE:

Based on the comments I have come up with the following solution.

function format_array {
    if [ -t 0 ]; then
        local ARRAY_IN=( $@ )
    else
        readarray ARRAY_ARGS < /dev/stdin
        local ARRAY_IN=( $@ ${ARRAY_ARGS[@]} )
    fi

    echo $(printf "%s\n" "${ARRAY_IN[@]}" | sort | normalize_paths | uniq)
}

shopt -s lastpipe
shopt -so pipefail

DEP_OUT=( $(changes "$DEP" "${DEP_OUT[*]}" "${DEP_HIERARCHY[*]}" | format_array "${DEP_OUT[*]}") ) && \
DEP_CHANGED=true

This was essential to getting the correct return code propagated.

(I thought one of the comments mentioned this briefly, it looks to have been removed)

shopt -s lastpipe
shopt -so pipefail
0

2 Answers 2

1

You want to simplify the code as used inside a recursive function.

The part that you want improved is the capture of the exit code of the function.

You should recognize that a return code is not the only way to transmit information. In fact, the fastest way to transmit information is with a variable. In a recursive function it should be a global variable (not defined with local) so the parent could read the value back. In this case, the line that sets a return code of 1 is

# return 1 (false) if $DEP_CHANGED = false && $DEP_FILE is cached
[ "$DEP_CHANGED" = false ] && (cached "$(dirname "$DEP_FILE")") && return 1

which could be changed to:

# Inform that data didn't change
[ "$DEP_CHANGED" = false ] && 
    (cached "$(dirname "$DEP_FILE")") && 
    dep_changed_in_function=false

And, correspondingly:

for DEP in "${DEP_LIST[@]}"; do
    dep_changed_in_function=true

    # find new deps
    DEP_OUT=( "$(changes "$DEP" "${DEP_OUT[*]}" "${DEP_HIERARCHY[*]}")" )
    DEP_OUT=( $(printf "%s\n" "${DEP_OUT[@]}" | sort | trim | uniq) )

    # set $DEP_CHANGED flag
    [ "$dep_changed_in_function" -eq true ] && DEP_CHANGED=true
done

But the second call to process the array DEP_OUT could be easily avoided if the data output of changes is already sorted.

That could be easily done by:

echo $(printf '%s\n' "${DEP_OUT[@]}" | sort | trim | uniq )

Instead of

echo "${DEP_OUT[@]}"

With that, the part of the code from your question reduces to:

for DEP in "${DEP_LIST[@]}"; do
    dep_changed_in_function=true

    DEP_OUT=( "$(changes "$DEP" "${DEP_OUT[*]}" "${DEP_HIERARCHY[*]}")" )

    # set $DEP_CHANGED flag
    DEP_CHANGED=$dep_changed_in_function
done

Of course, dep_changed_in_function=true could be set at the start of the function.

5
  • changes is recursive. the line DEP_OUT=( $(printf "%s\n" "${DEP_OUT[@]}" | sort | trim | uniq) ) does exact that Sep 30, 2019 at 21:48
  • I added a link to the gist in the main topic Sep 30, 2019 at 21:48
  • Yes, changes is recursive (and I am probably missing half of what happens inside it without an extensive description or adding many missing comments in the code) but at the end it builds DEP_OUT via a simple echo "${DEP_OUT[@]}", that could be modified to be echo "${DEP_OUT[@]}" | sort | trim | uniq.
    – user232326
    Sep 30, 2019 at 22:31
  • due to the recursion $DEP_OUT must be assigned once per loop, opposed to after all loops have completed Oct 2, 2019 at 23:12
  • My suggestion is executed once per loop (inside changes).
    – user232326
    Oct 2, 2019 at 23:16
0

Seen this before. Yes, it is kind of annoying.
The trick is to grab the return code of changes before executing any other command that may alter the return code.

I would try something like this:
DEP_OUT=( "$(changes "$DEP" "${DEP_OUT[*]}" "${DEP_HIERARCHY[]}")"; rc=$? )
or
DEP_OUT=( "$(changes "$DEP" "${DEP_OUT[*]}" "${DEP_HIERARCHY[
]}")" );rc=$?

Then use $rc in place or $?
[ $rc -eq 0 ] && DEP_CHANGED=true

If this works, you can then combine everything.
DEP_OUT=( "$(changes "$DEP" "${DEP_OUT[*]}" "${DEP_HIERARCHY[*]}";rc=$? )" | sort | trim | uniq)

Notice there are 3 different place where I've put the rc=$?. Depends on your shell, OS, instantiation, etc. You may have to experiment to find the exact right place to put it.

2
  • 1
    Good answer. While not the final solution I was able to combine your approach with shopt -s lastpipe shopt -so pipefail Oct 2, 2019 at 23:13
  • Glad I could help!
    – Scottie H
    Oct 2, 2019 at 23:15

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .