#3514 PR open: Fix parsing of workflow command line arguments

lzaoral opened issue at 2025-08-21 12:59:

Pull Request Details:

  • Type: Bug Fix

  • Impact: Critical

  • Reference to related issue (URL): https://github.com/rear/rear/pull/3500#issuecomment-3206558501

  • How was this pull request tested? rear format -- -y /dev/<DEVICE> on RHEL 8 and 10

  • Description of the changes in this pull request:
    Workflows expect that "$@" will not contain anything before the -- argument. Fixes the following failure:

$ rear format -- /dev/sdb
ERROR: Argument 'format' not accepted. Use 'rear format -- --help' for more information.

Partially reverts commit a2fe977d3cac54e1dcdad49146f822217c2b83d1.

pcahyna commented at 2025-08-21 13:57:

I think we should wait for @jsmeix to explain why he has moved the line in #3500 .

jsmeix commented at 2025-09-02 13:35:

Argh!

Now I see it why the part

# Keep the remaining command line arguments to feed to the workflow:
ARGS=( "$@" )

must be after the

# Set workflow to first command line argument or to "help" as fallback:
if test -z "$WORKFLOW" ; then
    # Usually workflow is now in $1 (after the options and its arguments were shifted above)
    # but when rear is called without a workflow there exists no $1 here so that
    # an empty default value is used to avoid 'set -eu' error exit if $1 is unset:
    if test "${1:-}" ; then
        # Not "$1" to get rid of compound commands:
        WORKFLOW=$1
        shift
    else
        WORKFLOW=help
    fi
fi

part.
Because therein $1 is used
so the remaining command line arguments
are those except the workflow command line argument.

I moved it up directly after the Parse options part
because I falsely thought the remaining command line arguments
are all after the options so it belongs there.

Now I think the proper ordering so that
what belongs together is kept together in the code
(cf. "Code must be easy to understand" in our "Coding Style")
should be:

# Parse options
help_note_text="Use '$PROGRAM --help' or 'man $PROGRAM' for more information."

...

    shift
done

# Set workflow to first command line argument or to "help" as fallback:
if test -z "$WORKFLOW" ; then
    # Usually workflow is now in $1 (after the options and its arguments were shifted above)
    # but when rear is called without a workflow there exists no $1 here so that
    # an empty default value is used to avoid 'set -eu' error exit if $1 is unset:
    if test "${1:-}" ; then
        # Not "$1" to get rid of compound commands:
        WORKFLOW=$1
        shift
    else
        WORKFLOW=help
    fi
fi

# Keep the remaining command line arguments to feed to the workflow:
ARGS=( "$@" )

# End of handling command line options and arguments.

# Set RECOVERY_MODE when we are running inside a rescue/recovery system
# which is normally ReaR's recovery system or alternatively in PORTABLE mode
# it is a foreign rescue system like the System Rescue CD or the Ubuntu Desktop Live CD,
# see doc/user-guide/17-Portable-Mode.adoc
# RECOVERY_MODE is a boolean variable (cf. conf/default.conf) because we need it below
# to set TMPDIR to ReaR's TMP_DIR only when we are not running inside a rescue/recovery system
# but we do not yet have lib/global-functions.sh (which contains is_true/is_false) sourced there.
# In ReaR's recovery system /etc/rear-release is unique (it does not exist otherwise)
# see build/default/970_add_rear_release.sh that adds it to identify ReaR's rescue environment.
# PORTABLE mode is meant to be used only to run 'rear recover' within a foreign rescue system,
# see https://github.com/rear/rear/pull/3206#issuecomment-2122173021
if test -e "/etc/rear-release" || test "$PORTABLE" ; then
    RECOVERY_MODE=1
fi

# Set alternative config directory unless in recovery mode
# because the contents of an alternative config directory
# get stored in the ReaR recovery system in /etc/rear
# so "rear recover" does not work with '-c'
# see https://github.com/rear/rear/issues/512#issuecomment-68051604
# and https://github.com/rear/rear/issues/2942#issuecomment-3132425914
if test "$ALTERNATIVE_CONFIG_DIR" ; then
    if test "$RECOVERY_MODE" ; then
@@ -326,26 +323,29 @@
    fi
    if ! test -d "$ALTERNATIVE_CONFIG_DIR" ; then
        # 'test -d something' is also true when something is a symlink to an existing directory
        echo "ERROR: The '-c' option argument '$ALTERNATIVE_CONFIG_DIR' is not a directory" >&2
        exit 1
    fi
    CONFIG_DIR="$ALTERNATIVE_CONFIG_DIR"
fi

jsmeix commented at 2025-09-02 13:39:

@lzaoral @pcahyna
do you agree to my proposed ordering of code parts in
https://github.com/rear/rear/pull/3514#issuecomment-3245380625
or is this again somehow wrong?

[Export of Github issue for rear/rear.]