blobモデルの実装 #3
blobモデルの実装 #3
Conversation
… into keigo-okamoto/feature-add-blob-model
| //追加でルールを指定 | ||
| rules: { | ||
| "no-unused-vars": ["error", { argsIgnorePattern: "^_.*$" }], | ||
| }, | ||
| languageOptions: { |
There was a problem hiding this comment.
_から始まる変数はunused扱いされたくないので，srcとdistどちらも無視するように設定しました
There was a problem hiding this comment.
ナイスです！こういう環境整えるの大事
あとでyotsu sanにもsyncしておきましょう！
めっちゃ厳密なこと言うとPR分けた方が良いかもしれませんが、個人的にはこのくらいのサイズのPRならシュッと混ぜちゃっててもいいかなって思いました
src/models/blob-object.ts
Outdated
| content?: Buffer; | ||
|
|
||
| public setContent = (content: Buffer): void => { | ||
| this.content = content; | ||
| }; |
There was a problem hiding this comment.
export class BlobObject {
content: Buffer;
constructor(content: Buffer) {
this.content = content;
}のようしてBlobObject 生成時にその中身も決定してしまうと content が undefined を取ることがなくなるため、メソッドの方から if (!this.content) を消せるかもしれない、と思いました
また、さらに踏み込むと content 自体が外部に露出している必要はないため
export class BlobObject {
constructor(private content: Buffer) {}で隠蔽してしまうと、一度作った BlobObject の中身をうっかり上書きすることがなくなりそうで、より安全かもです！
ref: https://typescriptbook.jp/reference/object-oriented/class/constructor-shorthand
There was a problem hiding this comment.
export class BlobObject {
constructor(private content: Buffer) {}この書き方初めて知りました！
おっしゃる通りだったので修正しました！
src/models/blob-object.ts
Outdated
| const hash = createHash("sha1").update(store).digest("hex"); | ||
|
|
||
| const dirPath = join(GIT_OBJECTS, hash.slice(0, 2)); | ||
| const filePath = join(GIT_OBJECTS, hash.slice(0, 2), hash.slice(2)); |
There was a problem hiding this comment.
この処理は git の object系では共通して出てきそうな処理なので、今後どこかのタイミングで切り出しても良いですね！このPRでは対応不要です！
src/models/blob-object.ts
Outdated
| new Uint8Array(Buffer.from(store)), | ||
| ); | ||
|
|
||
| if (existsSync(dirPath)) return; |
There was a problem hiding this comment.
この部分でディレクトリが存在する場合はblob objectの保存をスキップしているように見えますが、本来作成して欲しい時にもスキップ処理が走ってしまいそうです
例えば、過去に「aa123456789」というハッシュを持つファイルがあった場合
- aa
┗ 123456789
というファイルが存在しているはずです。このとき、新たに「aa00000000」というハッシュを持つ blob objectを作ろうと思うと
- aa
├ 0000000 <- New!
┗ 123456789
という状態にしたくなりそうです。しかし、今の処理だと「aa/ が存在するのでスキップ」となりそうです。
この場合、
- 追加対象のblob objectのハッシュから計算されたパスを持つファイルがあるなら早期return
- パスがないならblob objectを保存する。このときディレクトリがないのであれば作成する
というようにしておくと安心そうです！
There was a problem hiding this comment.
ありがとうございます！
修正しました！
| //追加でルールを指定 | ||
| rules: { | ||
| "no-unused-vars": ["error", { argsIgnorePattern: "^_.*$" }], | ||
| }, | ||
| languageOptions: { |
There was a problem hiding this comment.
ナイスです！こういう環境整えるの大事
あとでyotsu sanにもsyncしておきましょう！
めっちゃ厳密なこと言うとPR分けた方が良いかもしれませんが、個人的にはこのくらいのサイズのPRならシュッと混ぜちゃっててもいいかなって思いました
src/models/blob-object.ts
Outdated
| constructor(private readonly content: Buffer) {} | ||
|
|
||
| public dumpBlobObject = (): void => { | ||
| const store = `blob ${this.content.length.toString()}\x00${this.content.toString()}`; |
There was a problem hiding this comment.
すいませんこの部分見落としていました！
この方法でほとんど問題ないと思いますが、テキストファイル以外を取り扱おうとすると toStringできないバイナリデータが入ってきて正確な blob objectが作られなさそうです
適当な画像をmygit add してみたあと、git cat-file -p {hash値} > mygit_reverted.png のように戻して見た時、画像として表示されなくなりそうです
やったこと
blobモデルを実装した
詳細
Warning
オプション指定は本PRの対象外
確認項目
src/commands/add.tsにあるadd関数内のfilePathをlogするとハッシュが取得できます．