#3514 PR `merged`: Fix parsing of workflow command line arguments¶

Labels: bug, fixed / solved / done

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?

pcahyna commented at 2025-09-03 09:42:¶

@jsmeix before I look at it in detail, can you please explain what was wrong with the original (before the change in https://github.com/rear/rear/commit/a2fe977d3cac54e1dcdad49146f822217c2b83d1#diff-c88d8e622898fcfe3f447b77fe254dc6cc45a5152afa771e666be10971bfb01eR297 ) ordering that is proposed to be restored here?
I would like to know why it is needed to change it (from the original version) before reviewing the change.

jsmeix commented at 2025-09-03 13:25:¶

Nothing was "wrong" with the original
when "wrong" means "a bug in the code".

But the ordering was "wrong" when "wrong" means that
the ordering of code parts was confusing or misleading
because it intermixed different things
(at least as far as I can see currently).

Initially it was

# Parse options
...
# Set workflow to first command line argument or to "help" as fallback:
...
# Keep the remaining command line arguments to feed to the workflow:

There what belongs together is kept together in the code
i.e. the whole handling command line options and arguments.

Then I falsey inserted the new parts

# Set RECOVERY_MODE when we are running inside a rescue/recovery system
...
# Set alternative config directory unless in recovery mode

in between like

# Parse options
...
# Set RECOVERY_MODE when we are running inside a rescue/recovery system
...
# Set alternative config directory unless in recovery mode
...
# Set workflow to first command line argument or to "help" as fallback:
...
# Keep the remaining command line arguments to feed to the workflow:

and I noticed that the part

# Keep the remaining command line arguments to feed to the workflow:

belongs to

# Parse options

so I moved it up to the current mess which caused a bug
because currently the ordering is "wrong" where
"wrong" means "a bug in the code"

# Parse options
...
# Keep the remaining command line arguments to feed to the workflow:
...
# Set RECOVERY_MODE when we are running inside a rescue/recovery system
...
# Set alternative config directory unless in recovery mode
...
# Set workflow to first command line argument or to "help" as fallback:

Now - as far as I can see - the correct ordering should be

# Parse options
...
# Set workflow to first command line argument or to "help" as fallback:
...
# Keep the remaining command line arguments to feed to the workflow:
...
# Set RECOVERY_MODE when we are running inside a rescue/recovery system
...
# Set alternative config directory unless in recovery mode

i.e. keep the original three parts which are handling
command line options and arguments

# Parse options
...
# Set workflow to first command line argument or to "help" as fallback:
...
# Keep the remaining command line arguments to feed to the workflow:

together as it was initially
and afterwards add the two new parts

# Set RECOVERY_MODE when we are running inside a rescue/recovery system
...
# Set alternative config directory unless in recovery mode

Technically what is proposed in this pull request

# Parse options
...
# Set RECOVERY_MODE when we are running inside a rescue/recovery system
...
# Set alternative config directory unless in recovery mode
...
# Set workflow to first command line argument or to "help" as fallback:
...
# Keep the remaining command line arguments to feed to the workflow:

works - i.e. there is no bug - but as far as I can see
the two new parts do not belong in between the original three parts.

pcahyna commented at 2025-09-08 10:08:¶

@jsmeix I suppose this would be the complete diff that you propose:

diff --git a/usr/sbin/rear b/usr/sbin/rear
index e519ddb71..6cce0d18e 100755
--- a/usr/sbin/rear
+++ b/usr/sbin/rear
@@ -293,9 +293,25 @@ while true ; do
     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,
@@ -332,20 +348,6 @@ if test "$ALTERNATIVE_CONFIG_DIR" ; then
     CONFIG_DIR="$ALTERNATIVE_CONFIG_DIR"
 fi

-# 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
-
 # The following workflows are always verbose:
 case "$WORKFLOW" in
     (validate|shell|recover|mountonly)

This reordering looks good to me.

jsmeix commented at 2025-09-08 13:40:¶

@pcahyna
yes,
https://github.com/rear/rear/pull/3514#issuecomment-3265569945
is the complete diff I propose.

Should I make a pull request for this change
or would you or @lzaoral do it via this pull request here?

lzaoral commented at 2025-09-08 13:44:¶

@jsmeix No need to create a new PR, I'll rebase this one.

pcahyna commented at 2025-09-08 14:01:¶

By the way, I tried letting Claude Code analyze the proposed change.

I used the prompt

There is an uncommitted change in this repository, some code blocks are
reordered. The goal of the change is to improve readability, no functional
change is acceptable. Our task is to review the change. Is there really no
functional change caused by the reordering?

and the answer:

  Analysis of the code reordering:

  The code blocks that were moved:
  1. WORKFLOW setting block (lines 296-311) - moved earlier
  2. ARGS setting (line 311) - moved earlier

  The code that remained in its original position:
  1. RECOVERY_MODE setting (lines 313-326)
  2. CONFIG_DIR validation and setting (lines 328-347)

  Key dependency analysis:

  1. PORTABLE variable: This is set during option parsing (line 211:
  PORTABLE=1) and is used in the RECOVERY_MODE logic (line 324: if test -e 
  "/etc/rear-release" || test "$PORTABLE"). Since option parsing happens
  before both the original and new positions of the moved code, this
  dependency is preserved.
  2. ALTERNATIVE_CONFIG_DIR variable: This is set during option parsing
  (around line 221) and used in the CONFIG_DIR logic. Again, option parsing
  happens before the moved code.
  3. Variable dependencies within moved code: The WORKFLOW and ARGS setting
  only depends on $1 and $@, which are the command line arguments after
  option processing. This doesn't depend on RECOVERY_MODE or CONFIG_DIR.
  4. Variables used after the moved code: The case "$WORKFLOW" statement at
  line 350 still works correctly because WORKFLOW is now set earlier.

  Conclusion:

  The reordering appears to be functionally safe. The moved code blocks
  (WORKFLOW and ARGS setting) do not depend on RECOVERY_MODE or CONFIG_DIR
  settings, and all the variables they need ($1, $@) are available at the
  earlier position since they come from command line parsing which happens
  before both the old and new positions.

  The code that uses WORKFLOW (like the verbose workflow case statement)
  still works because WORKFLOW is set before it's used.

  This is indeed a pure readability improvement with no functional change.

I tried multiple times, the answer was similar in all except one, where Claude Code claimed that there was a difference in corner cases, but after a follow-up question it admitted that there is actually no difference.

It always correctly pointed out the problem in the current code.

jsmeix commented at 2025-09-08 15:19:¶

Now
https://github.com/rear/rear/pull/3514/files
looks exactly as I intended - i.e. the
Set workflow to first command line argument ...
block is moved up where it had been initially.

jsmeix commented at 2025-09-09 12:54:¶

@rear/contributors
unless there are objections
I would like to merge it on Thursday (11.Sept.2025) afternoon.

[Export of Github issue for rear/rear.]

#3514 PR merged: Fix parsing of workflow command line arguments¶