Some considerations about inifuncs.sh

[meleu]

The inifuncs.sh are really useful. It's a good tool to manage retroarch.cfg-like files, and it's better than build all those REGEXes everytime.

I'm posting this to give some feedback about using it to manage retroarch.cfg-like files and to share some thoughts.

[If I sounded like a smartass before, that was not my intention. Please take into consideration that English is not my native language and maybe I don't know how to joke using this idiom. No jokes from now on!]

Let's get started...

1. Is there a reason to get the last match in iniProcess?

In iniProcess the match variable allways gets the last occurrence of the key.

If iniSet/iniUnset is used to manage a retroarch.cfg file ("first match if finds" behavior), the comment character is put in the last match. This isn't what the caller expect. The comment on the first match is more like what RetroArch would do (or maybe it's just my opinion).

example:

[prompt]$ source ~/RetroPie-Setup/scriptmodules/inifuncs.sh                     
[prompt]$ iniConfig '=' '"'
[prompt]$ cat test.cfg 
var1="var1 content one"
var1="var1 content two"
var1="var1 content three"
[prompt]$ iniUnset var1 disabled test.cfg 
[prompt]$ cat test.cfg 
var1="var1 content one"
var1="var1 content two"
# var1="disabled"
[prompt]$ iniSet var1 'new value' test.cfg 
[prompt]$ cat test.cfg 
var1="var1 content one"
var1="var1 content two"
var1="new value"

2. The iniGet gets every occurrences of a key and puts their values in ini_value.

Thinking like RetroArch again. If the file has multiple occurrences of a key, the iniGet appends all the values in the ini_value variable. This can bring unexpected (and hard to detect) problems to the caller.

Again, I think RetroArch would get only the first match and ignore the rest.

Example:

[prompt]$ cat test.cfg 
var1="var1 content one"
var1="var1 content two"
var1="new value"
[prompt]$ iniGet var1 test.cfg 
[prompt]$ echo "$ini_value"
var1 content one
var1 content two
new value

I know the inifuncs.sh is not for retroarch.cfg manipulation only. So, I would like to know if you guys see any problem on making this "first match if finds" the default behavior.

[joolswills]

There shouldn't be duplicate keys - but these are not made specifically for retroarch so I don't want to make it use the first just for that. Why do you have duplicate keys ?

I will change iniGet to match the iniProcess behaviour.

[joolswills]

I have changed the iniGet to match iniProcess - which should protect against cases where people have manually edited a file - in regards to retroarch - if there is a specific case where something is breaking we can look into it. I might consider adding an additional parameter but I prefer to leave default behaviour as is.

[joolswills]

you could try

diff --git a/scriptmodules/inifuncs.sh b/scriptmodules/inifuncs.sh
index c34ee98..d03a644 100644
--- a/scriptmodules/inifuncs.sh
+++ b/scriptmodules/inifuncs.sh
@@ -14,11 +14,13 @@ function fatalError() {
     exit 1
 }

-# arg 1: delimiter, arg 2: quote, arg 3: file
+# arg 1: delimiter, arg 2: quote, arg 3: file, arg4: 1 to process first key rather than last when there are duplicates
 function iniConfig() {
     __ini_cfg_delim="$1"
     __ini_cfg_quote="$2"
     __ini_cfg_file="$3"
+    __ini_cfg_first_key="$4"
+    [[ -z "$__ini_cfg_first_key" ]] && __ini_cfg_first_key=0
 }

 # arg 1: command, arg 2: key, arg 2: value, arg 3: file (optional - uses file from iniConfig if not used)
@@ -41,8 +43,13 @@ function iniProcess() {
     local match_re="^[[:space:]#]*$key[[:space:]]*$delim_strip.*$"

     local match
+    local cmd=(egrep -i "$match_re" "$file")
     if [[ -f "$file" ]]; then
-        match=$(egrep -i "$match_re" "$file" | tail -1)
+        if [[ "$__ini_cfg_first_key" -eq 1 ]]; then
+            match=$("${cmd[@]}" | head -1)
+        else
+            match=$("${cmd[@]}" | tail -1)
+        fi
     else
         touch "$file"
     fi
@@ -117,7 +124,12 @@ function iniGet() {
         value_m="\(.*\)"
     fi

-    ini_value="$(sed -n "s/^[ |\t]*$key[ |\t]*$delim_strip[ |\t]*$value_m/\1/p" "$file" | tail -1)"
+    local cmd=(sed -n "s/^[ |\t]*$key[ |\t]*$delim_strip[ |\t]*$value_m/\1/p" "$file")
+    if [[ "$__ini_cfg_first_key" -eq 1 ]]; then
+        ini_value="$("${cmd[@]}" | head -1)"
+    else
+        ini_value="$("${cmd[@]}" | tail -1)"
+    fi
 }

 # arg 1: key, arg 2: default value (optional - is 1 if not used)

and add "1" parameter to iniConfig - if this works and is tested we could use this for when processing retroarch configs - but in many cases this is still not ideal as users may get confused when looking at their config when there are duplicates. You cannot really protect against users breaking configs when manually editing.

[meleu]

It's working fine at my first tests. I'll give you more feedback after a more intensive test.

Thanks.

[meleu]

[Just sharing an info about a sed issue I found (and already solved) when using iniSet (sed -i). I think it doesn't affect RetroPie-Setup because it runs with sudo.]

I saw a bunch of this error messages when using iniSet with ...all/retroarch.cfg:

sed: couldn't open temporary file /opt/retropie/configs/all/sedmSHx8G: Permission denied

I googled about this issue and found that it is a sed issue (bug?) that happens when the sed -i caller id doesn't have enough permissions to create files in the directory where the file it's trying to change is.

This is the case with user pi and the /opt/retropie/configs/all directory. The pi is the owner of the files and can edit them, but can't create new files in the directory.

In the sed info page (not the manpage) we have this info:

`-i[SUFFIX]'
`--in-place[=SUFFIX]'
     This option specifies that files are to be edited in-place.  GNU
     `sed' does this by creating a temporary file and sending output to
     this file rather than to the standard output.(1).
(...)

The problem happens because sed tries to create temporary file in the same directory of the file to be edited.

pi@retropie:/tmp $ mkdir dir
pi@retropie:/tmp $ sudo chown root.root dir
pi@retropie:/tmp $ cd dir
pi@retropie:/tmp/dir $ touch file
touch: cannot touch ‘file’: Permission denied
pi@retropie:/tmp/dir $ sudo touch file
pi@retropie:/tmp/dir $ sudo chown pi.pi file
pi@retropie:/tmp/dir $ echo I love my cat. > file
pi@retropie:/tmp/dir $ ls -lah
total 12K
drwxr-xr-x 2 root root 4.0K Aug 29 23:49 .
drwxrwxrwt 8 root root 4.0K Aug 29 23:49 ..
-rw-r--r-- 1 pi   pi     15 Aug 29 23:50 file
pi@retropie:/tmp/dir $ sed -i s/cat/dog/ file
sed: couldn't open temporary file ./sedf7B2WQ: Permission denied

[joolswills]

Yep - am aware of that - however the configs/all folder should be writeable by pi. There may have been an old bug where it was created with the wrong permissions, but I am not able to reproduce a situation currently where the permissions get set wrong. RetroPie-Setup also makes sure they are correct on launch.