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

ソースファイル末尾の空行を一括削除する #1170

Conversation

berryzplus
Copy link
Contributor

@berryzplus berryzplus commented Jan 24, 2020

PR の目的

ソースファイル末尾の空行を一括削除します。

削除することにより、空行に込められた意図を邪推する余地がなくなるので、「結果としてソースコードが分かりやすくなる」と考えています。

Gitクライアントには、ファイル末尾の空行を検知して警告する機能がついています。
オプション設定なので「有効にしなきゃいいだけ」ではありますが、
警告が出るようなファイルをリポジトリに入れとくのもキモいので一括で対応してしまいたいと思います。(追記:デフォルトで有効なオプションで、git diff -checkすると警告が出るようです。)

カテゴリ

  • リファクタリング

PR の背景

#1168 (comment)

EOF] 行を削除した結果、ファイルの末尾が空行になっているファイルがいくつもありますが、ファイル末尾の空行は削除した方がいいかもしれません。(新規でそのようなファイルを追加するとgitがwarningを出すはず。) #1169 も同様。

  ↓

末尾の空行は意図して残しました。

もしかしたらインクルードガードを部分的に復活させないといけないかもしれないんです。

  ↓

もしかしたらインクルードガードを部分的に復活させないといけないかもしれないんです。

うーん、復活させる必要が出てきたときに空行も復活させればいいんじゃないのかな?

  ↓

うーん、復活させる必要が出てきたときに空行も復活させればいいんじゃないのかな?

う、たしかにw

PR のメリット

自明なので略。

PR のデメリット (トレードオフとかあれば)

あとで必要になるかも知れないものも含めて、hとcppのすべての末尾空行を削除します。
⇒ 必要になったときに増やせばいい、ってことで特に問題はないと思っています。

PR の影響範囲

  • アプリ(=サクラエディタ)の機能に影響はありません。
  • ソースコードの見映えに影響する変更です。

関連チケット

#1168
#1169

参考資料

https://qiita.com/sgyt/items/a3151ec01815f9151de5

@berryzplus
Copy link
Contributor Author

PR作成時に叩いたコマンド

$array = Get-ChildItem . -File -Include *.h -Recurs -Name
foreach($file in $array) {
  (Get-Content -Path $file -Encoding UTF8BOM | out-string).Trim() | Set-Content -Path $file -Encoding UTF8BOM
}
$array = Get-ChildItem . -File -Include *.cpp -Recurs -Name
foreach($file in $array) {
  (Get-Content -Path $file -Encoding UTF8BOM | out-string).Trim() | Set-Content -Path $file -Encoding UTF8BOM
}

@beru
Copy link
Contributor

beru commented Jan 24, 2020

Gitクライアントには、ファイル末尾の空行を検知して警告する機能がついています。

具体的にどの git クライアントのどの設定なんでしょうか?

@AppVeyorBot
Copy link

@k-takata
Copy link
Member

具体的にどの git クライアントのどの設定なんでしょうか?

標準のgitコマンドで、差分を表示するときに--checkオプションを付けると空白のエラーをチェックしてくれます。

$ git diff --check --cached
foo.txt:3: new blank line at EOF.

(git show --check, git log --check なども使えます。)

@berryzplus
Copy link
Contributor Author

Gitクライアントには、ファイル末尾の空行を検知して警告する機能がついています。

具体的にどの git クライアントのどの設定なんでしょうか?

Git for Windows 以外のGitクライアントを知らんです。

世の中にはたくさんのGUIツールが存在しますが、
コマンドプロセッサは同じものを使っていると思ってました。
(git.exeを自作している人を除く・・・w)

Git の doc book にぼんやりとした説明がありました。

Git には、空白文字に関する問題を見つけて修正するための設定もあります。 空白文字に関する主要な六つの問題に対応するもので、そのうち三つはデフォルトで有効になっています。残りの三つはデフォルトでは有効になっていませんが、有効化することもできます。

デフォルトで有効になっている設定は、行末の空白文字を見つける blank-at-eol 、ファイル末尾の空白文字を見つける blank-at-eof 、行頭のタブ文字より前にある空白文字を見つける space-before-tab です。

実際どういう動きになるか試していませんが、
blank-at-eof を設定すれば警告なりエラーが出るはずです。

調べてみた感じ、主に git diff -check で問題が出る話のような印象を受けました。
https://stackoverflow.com/questions/27059239/git-new-blank-line-at-eof

ファイル内の改行については2つの考え方があるっぽいです。

  • End Of Line 派
    • newline 記号(=\n) は行を終端する、という考え方。(C文字列のNUL終端と似ている)
    • 最終行は newline 記号(=\n) 単独になる。
    • テキストファイルを定義するRFCはこの方式。いわゆる、POSIX方式。
  • Line Separator 派
    • ドキュメントを改行記号で分割したものが行であるという考え方。
    • 最終行の中身はなんでもよい。(newline 記号(=\n) を含んでいる必要はない。)
    • .NET Core の仕様はこっちの方式。

サクラエディタ内部は、基本的にEOL方式なんだけど、
最終行の仕様だけ「NL単独でなくてもよい」なので、
過去混乱が生じてEOL記号の描画まわりがぐちゃぐちゃしています。

このPRでその辺の技術仕様を整理したいかというとそうではないので、
単純に「末尾空行を削ったらコードがスッキリするぜ」で判断してもらえればよいかと思います。

@beru
Copy link
Contributor

beru commented Jan 25, 2020

標準のgitコマンドで、差分を表示するときに--checkオプションを付けると空白のエラーをチェックしてくれます。

$ git diff --check --cached
foo.txt:3: new blank line at EOF.

(git show --check, git log --check なども使えます。)

確認方法ありがとうございます。自分の方でも確認しました。

確認に使用した git クライアント

$ git --version
git version 2.17.1.windows.2

git diff--check オプションですが、設定の core.whitespace を見ると書かれています。
https://git-scm.com/docs/git-diff#Documentation/git-diff.txt---check

その設定について調べてみました。
https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration

下記の6つの設定が有ります。

設定名 内容
blank-at-eol 行末の空白駄目だよ
blank-at-eof ファイル末の空白駄目だよ
space-before-tab タブの前の空白駄目だよ
indent-with-non-tab 非タブでインデント駄目だよ
tab-in-indent タブでインデント駄目だよ
cr-at-eol CRを行末に使ってOKだよ

先頭の3つは初期状態で有効で、後半の3つは初期状態では有効になっていません。
git config コマンドで設定したり .git/config ファイルを書き換えて設定は出来ます。
[core] の whitespace にカンマ区切りで6つの設定を書けます。
OFFにしたい設定は先頭にハイフン文字を付けます。
indent-with-non-tabtab-in-indent は排他で両方を同時に有効にする事は出来ません。

Windowsで普段テキストファイルを作成すると改行コードがCRLFの事が多いですが、cr-at-eol 設定を有効化しないと、git diff --check すると trailing whitespace. 警告が出力されます。元々Linux向けのソフトだからですかね?改行コードの扱いは .gitattributes ファイルで制御できるし、人によっては core.autocrlf = true 設定を使ったりもするし、自分が普段使っている TortoiseGit だと警告をいちいち出さないのでこのエラーの存在を知りませんでした。

さて、ソースファイル末尾の空行に関連している設定ですが、blank-at-eof です。手元で確認してみるとファイル末尾に空行が2行以上存在していると git diff --check コマンド実行時に new blank line at EOF. というメッセージが出力されます。これも TortoiseGit だと(ry

@beru
Copy link
Contributor

beru commented Jan 25, 2020

具体的にどの git クライアントのどの設定なんでしょうか?

Git for Windows 以外のGitクライアントを知らんです。

世の中にはたくさんのGUIツールが存在しますが、
コマンドプロセッサは同じものを使っていると思ってました。
(git.exeを自作している人を除く・・・w)

自分は普段 TortoiseGit 経由で使う事が多いんですが、これは Git for Windows の git.exe を使ってるみたいですね。libgit2 を出来るだけ使わせる UseLibgit2 という設定もありますが、使った事無いです。昔は糞でかいリポジトリを扱うのには重過ぎて難が有ったんですがいつのまにかマシになったような…。

Git の doc book にぼんやりとした説明がありました。

Git には、空白文字に関する問題を見つけて修正するための設定もあります。 空白文字に関する主要な六つの問題に対応するもので、そのうち三つはデフォルトで有効になっています。残りの三つはデフォルトでは有効になっていませんが、有効化することもできます。
デフォルトで有効になっている設定は、行末の空白文字を見つける blank-at-eol 、ファイル末尾の空白文字を見つける blank-at-eof 、行頭のタブ文字より前にある空白文字を見つける space-before-tab です。

実際どういう動きになるか試していませんが、
blank-at-eof を設定すれば警告なりエラーが出るはずです。

調べてみた感じ、主に git diff -check で問題が出る話のような印象を受けました。
https://stackoverflow.com/questions/27059239/git-new-blank-line-at-eof

設定の説明読んでるだけだと挙動がよくわからないので実際に確認してみました。

#1170 (comment)

ファイル内の改行については2つの考え方があるっぽいです。

  • End Of Line 派

    • newline 記号(=\n) は行を終端する、という考え方。(C文字列のNUL終端と似ている)
    • 最終行は newline 記号(=\n) 単独になる。
    • テキストファイルを定義するRFCはこの方式。いわゆる、POSIX方式。
  • Line Separator 派

    • ドキュメントを改行記号で分割したものが行であるという考え方。
    • 最終行の中身はなんでもよい。(newline 記号(=\n) を含んでいる必要はない。)
    • .NET Core の仕様はこっちの方式。

サクラエディタ内部は、基本的にEOL方式なんだけど、
最終行の仕様だけ「NL単独でなくてもよい」なので、
過去混乱が生じてEOL記号の描画まわりがぐちゃぐちゃしています。

このPRでその辺の技術仕様を整理したいかというとそうではないので、
単純に「末尾空行を削ったらコードがスッキリするぜ」で判断してもらえればよいかと思います。

ファイル末尾の改行ですが1行は余白が有った方が良いという人もいなくも無いのか、1行だけの改行であればデフォルトで有効になっている blank-at-eof でも警告は出ませんでした。ただファイル末尾に改行が2行以上有るとそれは冗長とみなされるらしく警告が出ます。

CLI の git は軽くて良いんですが出力内容があまり視覚的にリッチではないので、最近は必要に迫られない限り普段使っていません。この警告の存在に今まで気づいてませんでした。まぁ個人的にはぶっちゃけどうでもいい問題な気がしますが、そんな事を言うとこだわりの塊のような人に 歯を食いしばれ と圧迫されボディブローを打たれそうな気が…。

@berryzplus
Copy link
Contributor Author

まぁ個人的にはぶっちゃけどうでもいい問題な気がしますが、そんな事を言うとこだわりの塊のような人に 歯を食いしばれ と圧迫されボディブローを打たれそうな気が…。

技術的根拠は「ついで」です。

メインの主張は「意味があるかどうか分からないファイル末尾の空行を削除すれば、空行に込められた意図を邪推する必要がなくなる結果、コードを分かりやすくできるんじゃないか」ということです。

もしかしたらインクルードガードを部分的に復活させないといけないかもしれないんです。

あと、これですが、今日検証してみた結果、キャンセルになりそうです。
g++ -c StdAfx.h するときのカレントディレクトリがあってないだけの話だったので。

@k-takata
Copy link
Member

ファイル末尾の改行ですが1行は余白が有った方が良いという人もいなくも無いのか、1行だけの改行であればデフォルトで有効になっている blank-at-eof でも警告は出ませんでした。ただファイル末尾に改行が2行以上有るとそれは冗長とみなされるらしく警告が出ます。

POSIX的には行は\nで終わることになっているので、ファイル末尾が1個の改行文字で終わっているならばそれは最終行の一部なので警告は出ません。ファイル末尾に2個以上の改行文字があるならば、1行以上の空行があるとみなされて警告が出ます。
逆に、ファイル末尾が\nで終わっていない場合、git diffを実行すると、\ No newline at end of fileと表示されます。(なぜかgit diff --checkは文句を言いませんが)

$ git diff --cached
diff --git a/foo.txt b/foo.txt
new file mode 100644
index 00000000..19102815
--- /dev/null
+++ b/foo.txt
@@ -0,0 +1 @@
+foo
\ No newline at end of file

GitHub上だと、代わりに0↵のような表示が出たはずです。

@beru
Copy link
Contributor

beru commented Jan 25, 2020

POSIX的には行は\nで終わることになっているので、ファイル末尾が1個の改行文字で終わっているならばそれは最終行の一部なので警告は出ません。ファイル末尾に2個以上の改行文字があるならば、1行以上の空行があるとみなされて警告が出ます。
逆に、ファイル末尾が\nで終わっていない場合、git diffを実行すると、\ No newline at end of fileと表示されます。(なぜかgit diff --checkは文句を言いませんが)
.....
GitHub上だと、代わりに0↵のような表示が出たはずです。

berryzplus さんの説明にも有りましたが、行が \n で終わるのって POSIX 由来なんですね。

ファイル末尾が改行コードで終わっていない問題の方がよりメジャーですね、たまにファイルの最終行に改行入れない人がたまにいて…。gcc は警告出してくれますね。ファイル終端が改行で終わっていないとCプリプロセッサで #include する時の繋がり方に影響出そうな気がするので必要だね…と自分は理解してました。

なお、最終行が改行コード付きの空行で終わっている場合でも、git diff --check で警告は出ていません。test.txt

\n
#include <stdio.h>\n
\n
int main(int argc, char* argv[])\n
{\n
  return 0;\n
}\n
\n

k-takata さんの説明だとこれも警告が出るような認識に見えますが少なくとも自分の環境では出ないですね。。おま環なんでしょうか…。

そして最終行とその前の行も改行コードだけの空行の場合だと new blank line at EOF. という警告が出ます。test.txt

\n
#include <stdio.h>\n
\n
int main(int argc, char* argv[])\n
{\n
  return 0;\n
}\n
\n
\n

これまぁ…連続する改行コードは 二つで十分ですよ! 分かってくださいよ って事でしょうか…。

@beru
Copy link
Contributor

beru commented Jan 25, 2020

技術的根拠は「ついで」です。

メインの主張は「意味があるかどうか分からないファイル末尾の空行を削除すれば、空行に込められた意図を邪推する必要がなくなる結果、コードを分かりやすくできるんじゃないか」ということです。

ファイル末尾の余分な装飾とか改行、とかは別に調整されても実害は無いので良いと思いますが、インデントのスタイルを統一とかの方向性まで走らないといいなぁと思います。人の好みは千差万別だし、自動フォーマッタは出来が悪いし、こういう事に意識を振り向けるのってまるで孫正義の前髪のような空虚さを感じます。

過去に sakura-editor/management-forum#29 で色々コメントが出て意見が別れました。まぁ他の人がやりたいなら止められません…。

@k-takata
Copy link
Member

なお、最終行が改行コード付きの空行で終わっている場合でも、git diff --check で警告は出ていません。test.txt

自分のところだと出ますね。

@beru
Copy link
Contributor

beru commented Jan 26, 2020

なお、最終行が改行コード付きの空行で終わっている場合でも、git diff --check で警告は出ていません。test.txt

自分のところだと出ますね。

自分のところでも出せるようになりました。

git diff --check だと変更前後の差分を見る為か、新規に add したファイルに対しては出ないみたいです。

下記のような場合に警告が出る事を確認しました。

  • 変更前ファイル終端空行無し → 変更後ファイル終端空行1行
  • 変更前ファイル終端空行1行 → 変更後ファイル終端空行2行

git log --check もコミット毎に前と比較したメッセージ出してるみたいなので、現状のワーキングコピー単体で各ファイルがどうなのかの列挙させる方法は良く分からずです。。

@berryzplus
Copy link
Contributor Author

なんだろな・・・。

「この場合は付ける」、「この場合は付けない」みたいにルール細分化するより、
気付いた時点で「えいやっ!」って対応してしまうのが分かりやすいのかな、と思ってます。

ただ、色々整理してきた中で今回出したPRの対応ってPOSIX方式じゃないなって気付いて「う~む」となってる感じです。

POSIX方式のファイル末尾

データがある最終行\n
\n (←末尾行は \n 単独でなければならない)
^Z  (←EOFの代わりです :smile:

このPRのファイル末尾

データがある最終行\r\n
^Z  (←EOFの代わりです :smile:

\n\r\n の違いは Cランタイムがうまいことやってくれると思ってよいはず。
形式的な違いは、ファイル末尾に空行があるかどうかです。
POSIX形式に従うと「1つ以上の空行がないといかん」のですけど、
このPRでは「空行が1つもない」にしてしまっていますね。

少し考えたいので、
とりあえず「ファイル末尾に空行が1つだけ存在する」を作ってみようかと思います。

@k-takata
Copy link
Member

新規に add したファイルに対しては出ないみたいです。

--cached を付ければ出ます。

@k-takata
Copy link
Member

POSIX方式のファイル末尾

\n (←末尾行は \n 単独でなければならない)

違います。それだと警告が出ます。
今のPRの内容で正しいです。

@beru
Copy link
Contributor

beru commented Jan 26, 2020

新規に add したファイルに対しては出ないみたいです。

--cached を付ければ出ます。

あ、本当ですね。

git diff だと index と working tree 間の差分で、git diff --cached だと index と HEAD 間の差分なんですね。

https://qiita.com/hide/items/17b970c485e803cbce08#%E3%81%BE%E3%81%A0-index-%E3%81%AB-add-%E3%81%97%E3%81%A6%E3%81%84%E3%81%AA%E3%81%84%E3%83%A2%E3%83%8E%E3%82%92%E8%A1%A8%E7%A4%BA

@berryzplus
Copy link
Contributor Author

POSIX方式のファイル末尾

\n (←末尾行は \n 単独でなければならない)

違います。それだと警告が出ます。
今のPRの内容で正しいです。

了解です。
それならこのままマージしてしまおうと思います。

@berryzplus berryzplus merged commit b819873 into sakura-editor:master Jan 26, 2020
@berryzplus berryzplus deleted the feature/remove_empty_lines_at_the_end_of_file branch January 26, 2020 05:14
@beru
Copy link
Contributor

beru commented Jan 26, 2020

「この場合は付ける」、「この場合は付けない」みたいにルール細分化するより、
気付いた時点で「えいやっ!」って対応してしまうのが分かりやすいのかな、と思ってます。

違いを気にする人はルール決めて統一したくなるんでしょうね。

スペース派 vs タブ派 のように有る意味どうでも良い事で論争が生まれるのはよくありますね。人によっては未だに1行80文字じゃないと嫌だと主張してくる事もあり窮屈な世の中だなと感じます。

@berryzplus
Copy link
Contributor Author

誤解はしてないと思いますが、やりたかったのは根拠不明なレガシーの排除です。

新規で入れるものに対して制約をかける内容じゃないし、
将来これと反する変更が入っても別によいと(ぼくは)思っています。

たとえばCLangに対応しようとしたら空行1個ないとダメだった(事実無根)とか、
他の人に説明できる理由があって、git diff -check で警告が出るトレードオフをペイできるなら
空行いっぱいつける方向にシフトしてもいいと思っています。

あるかも知れない主張の根拠
サクラエディタのスクロール範囲はファイル先頭~ファイル末尾+1行の範囲なので、
行末にある程度たくさんの改行がないと閲覧するときに見辛い場合がある。
  ↓
アプリ直せよ!と突っ込める主張だと思います。
(本体に入れてませんが、直したことあったはずです。)

末尾改行の有無の話とは異なり、
1行80桁の制約の根拠は、現代人にも理解できると思います。
 ⇒ 標準的なターミナルの表示桁数が80桁になっているから。

リモートホストにターミナルで入って作業する場合には、
現代でも普通に影響を受ける制約だと思っています。
(cat や type、Get-Content したときに、変なところで改行されて見辛い)

なので、こういう話をちゃんと理解してる人が言う、
シェルスクリプトは1行あたり80桁までとすべきだ!
という主張には同意できるんです。

初めてコンピューター触ったときから時間が止まってるおっさんの言う、
テキストファイルは1行あたり80文字までとすべきだ!
という主張には「はぁ?」と訊き返すことになるんです。

ただの妄信と、一定の根拠に基づいた選択は、違うと思っています。
できるだけ、ただの妄信は排除していきたいです。

自分のポリシーが「ただの妄信」なのか、違うのか、
それを知ることには大きな意味があると思います。
それを知ることで、理解レベルが1ランク上がることになるような気がするので。

@beru
Copy link
Contributor

beru commented Jan 26, 2020

うーん、C++ で書かれてるソースコードが1行80桁とか結構無理がある気がするんですけど…。

@berryzplus
Copy link
Contributor Author

うーん、C++ で書かれてるソースコードが1行80桁とか結構無理がある気がするんですけど…。

まぁ、状況により、です。

C++ソースコードをターミナル経由で編集してビルドする機会はめったにない・・・
(というか、仮にそんな事態になったら、リリース中止ないし保守対応不能ですよ。)
なので、C++ソースコードには1行80桁ルールを適用する実益はないと思います。

C++ソースコードは 1行あたり80桁 収めるべきだ!
って主張にはたぶん「なんでやねん!」と突っ込む気がします。

理由があるならそれを吟味するし、理由がないならそれは放置するだけっす。

@beru
Copy link
Contributor

beru commented Jan 26, 2020

意見が分かれる場合にどう折り合いを付けるか的な話にもなりますが、自分とは意見が一致しない相手がいた場合、その人はその人で多分ちゃんとした理由があるとは思うんですよね。それが相手に伝わるようにちゃんと表現しているかはコミュニケーションの話なのでおいておいて。

で、仮に誰かが C++だけど、かくかくしかじかだから1行80文字にしたいっていうかするわ。お前ら俺に合わせろ と言われた場合は幽体離脱を体験するチャンスだとポジティブに捉えたいと思います。

@KENCHjp KENCHjp added the refactoring リファクタリング 【ChangeLog除外】 label Feb 4, 2020
@m-tmatma m-tmatma added this to the v2.4.0 milestone Apr 19, 2020
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 16, 2020
…empty_lines_at_the_end_of_file

ソースファイル末尾の空行を一括削除する
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring リファクタリング 【ChangeLog除外】
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants