Skip to content
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.

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

Make @sh work with objects; add @shassoc (fix #1947) #2828

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nicowilliams
Copy link
Contributor

@nicowilliams nicowilliams commented Aug 6, 2023

  • mention eval in @sh docs
  • make @sh work on objects, emitting shell variable assignments for ident-like keys
  • add @shassoc that emits text that can be used with declare -A x; eval x=($(jq -r ...))

@nicowilliams nicowilliams force-pushed the fix_1947 branch 2 times, most recently from 1b34512 to 4617ee3 Compare August 6, 2023 22:56
@emanuele6
Copy link
Member

emanuele6 commented Aug 7, 2023

The examples are wrong; they are using unquoted expansions that will split the output by whitespace, and then eval will join the splits back collapsing a sequence of multiple spaces into a single space. also globs will be expanded.

declare -A x; eval x=\($(jq -r '@shassoc' <<< '{"foo   bar": "x  /*   y"}')\)

$(jq -r '@shassoc' <<< '{"foo bar": "x /* y"}') will expand into to ['foo bar']='x /* y' since it is unquoted, it will be split into:

<x=(['foo> <bar']='x> </*> <y')>

And again, since it is unquoted, /* will expand, ending up running:

eval 'x=(['\''foo' 'bar'\'']='\''x' '/bin' '/etc' '/...' 'y'\'')'

That will eval x=(['foo bar']='x /bin /etc /... y'), which is wrong: all spaces collapsed into one, and /* is randomly expanded. Also /* may expand to something that injects syntax and runs a command.

Even for simple cases, [foo] is a glob pattern that matches either f or o, using an unquoted expansion will make it expand.

Anyway, sorry, but even if you fix the problem, I don't really like this implementation at all. And I don't think even think this is useful, or that we should add it.

Do you really want to add this feature?

@nicowilliams
Copy link
Contributor Author

nicowilliams commented Aug 7, 2023

: ; declare -A x
: ; eval x=($(./jq -nr '{"a b":"*"}|@shassoc'))
: ; echo ${#x[@]} "${x[@]}"
1 *
: ; echo ${#x[@]} "${!x[@]}" "${x[@]}"
1 a b *
: ; eval x=($(./jq -nr '{"*":"*"}|@shassoc'))
: ; echo ${#x[@]} "${!x[@]}" "${x[@]}"
1 * *
: ; 

@emanuele6
Copy link
Member

emanuele6 commented Aug 7, 2023

@nicowilliams

It only makes sense that x=(['*']='*') doesn't glob expand to the files in your directory, the are no spaces so it is a glob in its entirety. It would expand to files named x=(*='foo') or x='=('foo bar' baz') in your current directory if there were any (and it would expand to nothing if nullglob was in use, like ['foo']='bar' also would since [] is a glob component too).
And "a b" does not have sequence of multiple spaces so you don't notice that the sequences of spaces are collapsed into a single space by eval joining them back after splitting.

$ (declare -A x; eval x=($(./jq -nr '{"*":"*"}|@shassoc')); declare -p x)
declare -A x=(["*"]="*" )
$ touch "x=(*='foo' bar')" "x=('='x')"
$ (declare -A x; eval x=($(./jq -nr '{"*":"*"}|@shassoc')); declare -p x)
declare -A x=(["*=foo"]="bar) x=(=x" )
$ (declare -A x; eval "x=($(./jq -nr '{"*":"*"}|@shassoc'))"; declare -p x) #quoted
declare -A x=(["*"]="*" )

You can also check my example:

$ (declare -A x; eval x=\($(./jq -r '@shassoc' <<< '{"foo   bar": "x  /*   y"}')\); declare -p x)
declare -A x=(["foo bar"]="x /bin /boot /dev /etc /home /lib /lib64 /lost+found /mnt /opt /proc /root /run /sbin /srv /sys /tmp /usr /var y" )
$ (declare -A x; eval "x=($(./jq -r '@shassoc' <<< '{"foo   bar": "x  /*   y"}'))"; declare -p x) #quoted
declare -A x=(["foo   bar"]="x  /*   y" )

@nicowilliams
Copy link
Contributor Author

Oh, I see. Maybe then @shassoc should expand to var[index]=value with the index and value escaped? but whence the associative array variable name? From an empty key's value?

@emanuele6
Copy link
Member

You just need to double quote the expansion so it is not splitted and glob expanded: eval "x=($(jq ...))" instead of eval x=($(jq ...)); also note that you don't need to use eval and you can use declare directly in bash: declare -A "x=($(jq ...))".

Anyway, I am still of the opinion that this is not a good feature to add. And also note that bash (but not ksh93) disallows the use of '' as key for an associative array, and you are not handling that case which will result in an assignment error.

@nicowilliams
Copy link
Contributor Author

nicowilliams commented Aug 7, 2023 via email

the object that are valid shell variable names (other keys
will be ignored).

E.g., `eval $(jq -r '@sh' f.json)`
Copy link
Member

@emanuele6 emanuele6 Aug 7, 2023

Choose a reason for hiding this comment

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

I don't understand what this example is supposed to show, why would you run @sh to quote everything into a single quoted shell word, and use eval? It would be the exact same thing as running "$(jq -r . f.json)". But, anyway, it is still wrong.
You need to quote the expansion, and arbitrary strings passed to eval need to be prefixed by a space to avoid making the shell interpret a string that starts with - as an option to eval: eval " $(foo)"

Most of the times I use @sh, I don't use it with eval, I use it with | sh:

jq -r '.things[] | @sh "cmd -o \(.foo) -- \(.majigs)"' | sh

eval is only useful if you want to set multiple variables at once (if you want to set only one variable, you just use foo=$(jq)), or an indexed/associative array variable, though it is more convenient to use declare for that.

If we want to put an example, it should at least make sense.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, this is supposed to be an example for the new @sh behaviour for objects that creates a string of assignments.
e.g. jq -n '{foo:1, bar:2} | @sh' => foo='1' bar='2'.
You still need to use proper quoting to make this actually work, but at least now I get it. ^^

eval " $(jq -r '@sh' file.json)"

@emanuele6
Copy link
Member

Can you write `@sh` instead of @sh in your commit message so github does not render it as a user mention ^^

@xPMo
Copy link

xPMo commented Aug 20, 2023

I recently made a jq module to do some of this, including the functionality that was removed. With the goal of mapping objects to identifiers, sh::param has an optional prefix argument (default is _), so you don't clobber important shell parameters on accident, and can namespace multiple calls to the function.

I'm looking to find the best way to incorporate which $SHELL the user is running without turning the function into a messy 4x nested if-else tree.

Not disparaging a C builtin approach, but I just wanted to add this as option, especially if someone in the future found this PR from the issue search (like I did).

@nicowilliams
Copy link
Contributor Author

nicowilliams commented Aug 21, 2023

Not disparaging a C builtin approach, but I just wanted to add this as option, especially if someone in the future found this PR from the issue search (like I did).

No disparagement taken :)

By the way, your comment prompted me to see what it would take to add user-defined formatting specifications, and... it can be done right now:

def of($fmt): format($fmt); # original `format`
def format($fmt): if $fmt == "custom" then "CUSTOM: " + . else of($fmt) end;

@custom "hi \("there")"

Now, modules can't quite do this unless they're included instead of imported, but one could always document that the user of a module that defines formats can def format($fmt): the_module::format($fmt); to use them.

@wader
Copy link
Member

wader commented Aug 21, 2023

By the way, your comment prompted me to see what it would take to add user-defined formatting specifications, and... it can be done right now:

There is jaq PR 01mf02/jaq#105 discussion add formatters and custom formatters by allowing a function to start with "@" which seems quite nice.

@nicowilliams
Copy link
Contributor Author

There is jaq PR 01mf02/jaq#105 discussion add formatters and custom formatters by allowing a function to start with "@" which seems quite nice.

I like that, yeah.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants