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

Fix bashism #311

Merged
merged 3 commits into from
Mar 6, 2024
Merged

Fix bashism #311

merged 3 commits into from
Mar 6, 2024

Conversation

bastien-roucaries
Copy link
Contributor

Hi,

Could you please check and merge this ?

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

@bastien-roucaries
Copy link
Contributor Author

Note that it is an incomplete fix bashism

@Schievel1
Copy link
Collaborator

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
Copy link
Collaborator

Schievel1 commented Feb 19, 2024

@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
Copy link
Contributor Author

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

@Schievel1
Copy link
Collaborator

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
Copy link
Contributor Author

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

41_snapshots-btrfs Outdated Show resolved Hide resolved
41_snapshots-btrfs Outdated Show resolved Hide resolved
@Schievel1 Schievel1 merged commit 94d742d into Antynea:master Mar 6, 2024
1 check passed
@Schievel1
Copy link
Collaborator

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

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.

2 participants