| Message ID | 20200307172248.GA9@afee69d503a7 |
|---|---|
| State | New |
| Headers | show |
| Series | [v8,1/9] qemu-binfmt-conf.sh: enforce safe style consistency | expand |
On 3/7/20 11:22 AM, Unai Martinez-Corral wrote: > Spaces are added before '; then', for consistency. For consistency with what? This is not our prevailing style; as evidenced by this pre-patch search: $ git grep 'if \[.*\];' | wc 274 2186 18170 $ git grep 'if \[.*\] ;' | wc 25 256 1573 and you are diverging from the dominant pattern. > > All the tests are prefixed with 'x', in order to avoid risky comparisons > (i.e. a user deliberately trying to provoke a syntax error). This part, however, is good. Since one part is controversial, you may want to split this into two patches, or even drop the reformatting part.
2020/03/09 16:01, Eric Blake: > On 3/7/20 11:22 AM, Unai Martinez-Corral wrote: > > Spaces are added before '; then', for consistency. > > For consistency with what? This is not our prevailing style; as > evidenced by this pre-patch search: > > $ git grep 'if \[.*\];' | wc > 274 2186 18170 > $ git grep 'if \[.*\] ;' | wc > 25 256 1573 > > and you are diverging from the dominant pattern. > For consistency within the script that is being modified. I'm not trying to diverge, neither do I prefer any specific style. Although the style in the current master is not consistent, ' ; ' is significantly more frequent. When I was told to keep consistency in v2, I picked that because it was the most common. Anyway, I will push a new version where all these are changed to the dominant pattern outside of this script. > This part, however, is good. Since one part is controversial, you may > want to split this into two patches, or even drop the reformatting part. > Since the current master is neither consistent nor coherent with the dominant pattern, I don't think I can drop the reformatting as long as I want to fulfill your requirements.
On 3/9/20 1:20 PM, Unai Martinez Corral wrote: > 2020/03/09 16:01, Eric Blake: > >> On 3/7/20 11:22 AM, Unai Martinez-Corral wrote: >>> Spaces are added before '; then', for consistency. >> >> For consistency with what? This is not our prevailing style; as >> evidenced by this pre-patch search: >> >> $ git grep 'if \[.*\];' | wc >> 274 2186 18170 >> $ git grep 'if \[.*\] ;' | wc >> 25 256 1573 >> >> and you are diverging from the dominant pattern. >> > > For consistency within the script that is being modified. I'm not trying to > diverge, neither do I prefer any specific style. Aha, I see what you were looking at: within the script itself, it was 10 '] ;' vs. 2 '];'. In which case, I'd recommend swapping the 10 instances over to be common with the rest of the code base, rather than the 2 away from the rest of the code base but towards the rest of the script. > Although the style in the current master is not consistent, ' ; ' is > significantly more frequent. When I was told to keep consistency in v2, I > picked that because it was the most common. > Anyway, I will push a new version where all these are changed to the > dominant pattern outside of this script. Good to hear. > > >> This part, however, is good. Since one part is controversial, you may >> want to split this into two patches, or even drop the reformatting part. >> > > Since the current master is neither consistent nor coherent with the > dominant pattern, I don't think I can drop the reformatting as long as I > want to fulfill your requirements. Splitting into two patches (one to fix '] ;' spacing, the other to add '[ "x$..."' protection) is then the best course of action.
diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh index 9f1580a91c..672ce716b6 100755 --- a/scripts/qemu-binfmt-conf.sh +++ b/scripts/qemu-binfmt-conf.sh @@ -223,12 +223,12 @@ qemu_check_access() { qemu_check_bintfmt_misc() { # load the binfmt_misc module - if [ ! -d /proc/sys/fs/binfmt_misc ]; then + if [ ! -d /proc/sys/fs/binfmt_misc ] ; then if ! /sbin/modprobe binfmt_misc ; then exit 1 fi fi - if [ ! -f /proc/sys/fs/binfmt_misc/register ]; then + if [ ! -f /proc/sys/fs/binfmt_misc/register ] ; then if ! mount binfmt_misc -t binfmt_misc /proc/sys/fs/binfmt_misc ; then exit 1 fi @@ -259,10 +259,10 @@ qemu_check_systemd() { qemu_generate_register() { flags="" - if [ "$CREDENTIAL" = "yes" ] ; then + if [ "x$CREDENTIAL" = "xyes" ] ; then flags="OC" fi - if [ "$PERSISTENT" = "yes" ] ; then + if [ "x$PERSISTENT" = "xyes" ] ; then flags="${flags}F" fi @@ -300,18 +300,18 @@ qemu_set_binfmts() { mask=$(eval echo \$${cpu}_mask) family=$(eval echo \$${cpu}_family) - if [ "$magic" = "" ] || [ "$mask" = "" ] || [ "$family" = "" ] ; then + if [ "x$magic" = "x" ] || [ "x$mask" = "x" ] || [ "x$family" = "x" ] ; then echo "INTERNAL ERROR: unknown cpu $cpu" 1>&2 continue fi qemu="$QEMU_PATH/qemu-$cpu" - if [ "$cpu" = "i486" ] ; then + if [ "x$cpu" = "xi486" ] ; then qemu="$QEMU_PATH/qemu-i386" fi qemu="$qemu$QEMU_SUFFIX" - if [ "$host_family" != "$family" ] ; then + if [ "x$host_family" != "x$family" ] ; then $BINFMT_SET fi done