Qithub-BOT / QiiCipher

✅ GitHub の SSH 公開鍵でファイルを暗号化およびローカルの秘密鍵で復号・署名・検証するスクリプトのリポジトリです。
https://qiita.com/KEINOS/items/2abce1e5b15d799ac6d7
Creative Commons Attribution Share Alike 4.0 International
4 stars 3 forks source link

Fix issue 25 md5s #26

Closed KEINOS closed 3 years ago

KEINOS commented 3 years ago

Issue #25 を修正してみました。

md5s だけでなく md5f も同じ問題を孕んでますが、

Issue 発生 → Draft PR → 再現テスト(test: failure) → 不具合修正(test: success)→ 修正の反映 → PR

の流れを確認してみたかったので、md5s の修正に限定してみました。

お手すきにチェックお願いします。

yoshi389111 commented 3 years ago

確認しました。修正箇所は問題ないと思います。

1つ気になるのは、どうやらテストコード issue25_test.sh が bash ではなく sh で動いているように思えることです。

たとえば

md5s() {
    if [ -e "$(which md5sum)" ]; then
        md5sum <(echo "$1") | awk '{ print $1 }'
    # 以下略

のように、bash ではつかえるけど sh では使えない書き方( <(echo "$1") )を使っていると、エラーになります。 .github/run-test.sh で実行しても同様です。

shellspec --shell bash とすればテストは通ります。 同様に .shellspec--shell bash を足した場合もテストは通ります。

(テストケース1行目の shellcheck shell=bash が効いていない?)

KEINOS commented 3 years ago

確認しました。修正箇所は問題ないと思います。

確認あざっす!

確認ですが、こういった、なんちゃって TDD 的なフローはレビューしやすいですか?

issue25_test.sh が bash ではなく sh で動いているように思える

そうなんですよ!おっしゃるように、--shell bash と明示しないとダメらしく、テストケースのヘッダー行が無視されているようです。

bash ではつかえるけど sh では使えない書き方

確かに他のスクリプトはプロセス置換(?)ではなくパイプ渡し(echo "$1" | md5sum |)の書き方をしているので、合わせた方がいいですよね。修正します。

せっかく shellspec を使っているので、最終的には Bash 中心でなく POSIX 準拠にしたいのですが、とりあえず今は Bash で進めて、別 Issue をあげたいと思います。

yoshi389111 commented 3 years ago

なんちゃって TDD 的なフローはレビューしやすいですか?

うーん、どうでしょう。(役に立たない意見)

修正意図はわかりやすいかもしれないですね。

確かに他のスクリプトはプロセス置換(?)ではなくパイプ渡し(echo "$1" | md5sum | )の書き方をしているので、合わせた方がいいですよね。修正します。

いえ、直した方が良いという意味ではありません。 いろいろ確認していたら、bash ではないようだったので、説明上書き換えて、確認しただけです。 今のままで問題ないと思います。

最終的には Bash 中心でなく POSIX 準拠にしたいのですが

POSIX 準拠の方が移植性は良いだろうと思いますので、賛成です。

emadurandal commented 3 years ago

@KEINOS @yoshi389111 わわ、動かなかったですか。すみませんMacでの確認が足りなかったみたいですね💦

最終的には Bash 中心でなく POSIX 準拠にしたいのですが POSIX 準拠の方が移植性は良いだろうと思いますので、賛成です。

たしかにそうですね。私があまりシェルスクリプトに慣れていないもので、今後も私のコントリビュートについてはご面倒をおかけするかもしれません(勉強します)

KEINOS commented 3 years ago

わわ、動かなかったですか。

いやいや! ✋

十分な確認をせずにマージした私の落ち度なんです。私のアイコンをご覧ください。

ね?ザル🐒 なんですw。なので、テストを入れようとしているのは、私のポカよけの意味が多分にあるし、見つけられただけ良いことで直せばいいので気にしないでください。むしろ #5 の PR のおかげで重い腰が動いたのです。ありがとうございます。

POSIX 準拠の方が移植性は良いだろうと思いますので、賛成です。

POSIX 互換にしたいのは Docker で作業することが多くなったので、そこでも使えるとカッチョいいなというだけ(本音)で、大義としては、POSIX 準拠したスクリプトであればより幅広い人にも使えるかな、と。カッチョいいし。

とりあえず、変更せずに進めて、別 Issue 別 PR で POSIX 準拠を進めたいと思います。

各々がたチェックありがとうございます!

KEINOS commented 3 years ago

とりあえず、テスト時のシェルを bash にするように .shellspec ファイルに追記しました。