Skip to content

Add warning if run command has multiple arguments #187

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gportay wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add warning if run command has multiple arguments #187

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
199cdbf
cqfd: run: add warning if multiple arguments
gportay Jan 31, 2025
83636d1
cqfd: run -c: add warning if multiple arguments
gportay Dec 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion cqfd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,7 @@ while [ $# -gt 0 ]; do
has_to_release=true
fi

cqfd_command="$1"
shift

# No more args? run default command
Expand All @@ -701,13 +702,28 @@ while [ $# -gt 0 ]; do

if [ "$#" -lt 1 ]; then
usage
die "run -c: Missing arguments!"
die "$cqfd_command -c: Missing arguments!"
fi

# Display a warning message if using -c with multiple arguments
if [ "$#" -gt 1 ]; then
warn "command $cqfd_command -c shall support a single argument," \
"please consider the command run -c instead:"
echo " cqfd $cqfd_command -c \"$*\"" >&2
fi
break
fi

# Run alternate command
has_alternate_command=true

# Display a warning message if using run with multiple arguments
Copy link
Copy Markdown
Contributor

@florentsfl florentsfl Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this warning depends on if we really want to make cqfd run disappear totally or keep it for retrocompatibility

Copy link
Copy Markdown
Contributor Author

@gportay gportay Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes kind of. but there is a world before removing the command run and just deprecating it using a warning telling to switch to another command.

So up to you.

Copy link
Copy Markdown
Contributor

@florentsfl florentsfl Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but the question is: do we really want to make people forget the "run" keyword ? If we don’t want that, then it is not necessary to put a warning.

run is used everywhere so it will be cumbersome for people to change, especially since there is no real issue with it at the moment, it works sufficiently well for what we need.

Copy link
Copy Markdown
Contributor Author

@gportay gportay Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The run command does not work properly since cqfd run "touch foo" and cqfd run touch foo expect the same result. Since the command cannot forward the parameters property to the container, I suggest to move to another command that does it correctly.

This would prevent people to file an issue because it cannot deal with parameters:

cqfd run bash -c 'echo $0 $# $*' zero one two three
# no output because it runs bash -c echo $0 $# $* zero two three. the command string is word split.
#  sh -c [-abCefhimnuvx] [-o option]... [+abCefhimnuvx] [+o option]... command_string [command_name [argument...]]
# with command_string is echo instead of echo "$0 $# $*"
# arguments are: $0, $#, $*, zero, one, two, three instead of zero, one, two, three

The example below shows how the run command it broken.

So my opinion is to tell the user the command is broken and it should move to another one, as it does not work for every use case.

And we cannot fix it because the people using it as cqfd run "touch foo" would file an issue because it does not work anymore, and that example was part of the README.md.

So deprecating it is the only solution.

if [ "$#" -gt 1 ]; then
warn "command $cqfd_command with multiple arguments is ambigous," \
"please consider the commands exec or shell instead:"
echo " cqfd exec ${*@Q}" >&2
echo " cqfd shell -c \"${*@Q}\"" >&2
fi
break
;;
sh|ash|dash|bash|ksh|zsh|csh|tcsh|fish|shell)
Expand Down