Antynea / grub-btrfs

Include btrfs snapshots at boot options. (Grub menu)
GNU General Public License v3.0
773 stars 78 forks source link

Fix bashism #311

Closed bastien-roucaries closed 8 months ago

bastien-roucaries commented 10 months ago

Hi,

Could you please check and merge this ?

For debian we will like to only use dash for this package

bastien-roucaries commented 10 months ago

Note that it is an incomplete fix bashism

Schievel1 commented 9 months ago

Thanks for your contribution. Shouldn't the shebang then be /bin/sh as well?

https://github.com/Antynea/grub-btrfs/blob/465b56107f1a9a983e132d17441c2d63cdc16392/41_snapshots-btrfs#L1C1-L1C21

Schievel1 commented 9 months ago

@bastien-roucaries could you explain that a bit more? You haven't fixed all bashisms in this, yet you did enough for dash to work with this. Does that mean dash works with some bashisms, not all?

I still have this error when doing shellcheck -s sh:

In 41_snapshots-btrfs line 301:
        local path_snapshot=${snap[@]:13:${#snap[@]}}
        ^-----------------^ SC3043 (warning): In POSIX sh, 'local' is undefined.
                            ^-----------------------^ SC2068 (error): Double quote array expansions to avoid re-splitting elements.
                            ^-----------------------^ SC2124 (warning): Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.

When doing shellcheck -s dash I get tons of errors like this:

In 41_snapshots-btrfs line 548:
    name_microcode=("${list_ucode[@]##*"/"}")
                   ^------------------------^ SC3030 (error): In dash, arrays are not supported.
bastien-roucaries commented 9 months ago

@Schievel1 it is a first pass for eliminating bashism. For array I will need more review

Schievel1 commented 9 months ago

Ok, I get it. Then you will follow up with more PRs regarding this? This is a ton of work to work around the arrays here, isn't it?}

bastien-roucaries commented 9 months ago

@Schievel1 not necesseraily if we could get the code clearer by refactoring

Schievel1 commented 8 months ago

Thanks for your contribution. I am looking forward to test following PRs from you that purge bashisms :)