diff --git a/commit.go b/commit.go index f3229a8..158bcac 100644 --- a/commit.go +++ b/commit.go @@ -22,23 +22,22 @@ import ( ) // CommitInterface describes the methods which must be implemented by the -// different commit types. +// different commit types. None of the methods should modify the underlying +// object. type CommitInterface interface { // MessageHead returns the head of the commit message (i.e. the first line). // The CommitCommon of the outer Commit is passed in for added context, if // necessary. MessageHead(CommitCommon) (string, error) - // Hash returns the raw hash which Signifiers can sign to accredit this - // commit. The tree objects given describe the filesystem state of the - // parent commit, and the filesystem state of this commit. - // - // This method should _not_ change any fields on the commit. - Hash(parent, this *object.Tree) ([]byte, error) + // ExpectedHash returns the raw hash which Signifiers can sign to accredit + // this commit. The ChangedFile objects given describe the file changes + // between the parent commit and this commit. + ExpectedHash([]ChangedFile) ([]byte, error) - // GetHash returns the signable Hash embedded in the commit, which should - // hopefully correspond to the Commit's Credentials. - GetHash() []byte + // StoredHash returns the signable Hash embedded in the commit, which should + // hopefully correspond to the ExpectedHash. + StoredHash() []byte } // CommitCommon describes the fields common to all Commit objects. @@ -173,7 +172,7 @@ func (r *Repo) AccreditCommit(commit Commit, sigInt sigcred.SignifierInterface) return commit, fmt.Errorf("could not grab snapshot of HEAD fs: %w", err) } - cred, err := sigInt.Sign(headFS, commitInt.GetHash()) + cred, err := sigInt.Sign(headFS, commitInt.StoredHash()) if err != nil { return commit, fmt.Errorf("could not accredit change commit: %w", err) } @@ -388,18 +387,18 @@ func (r *Repo) verifyCommit(branch plumbing.ReferenceName, gitCommit GitCommit, } // assert access controls - filesChanged, err := calcDiff(parentTree, gitCommit.GitTree) + changedFiles, err := ChangedFilesBetweenTrees(parentTree, gitCommit.GitTree) if err != nil { return fmt.Errorf("calculating diff from tree %q to tree %q: %w", parentTree.Hash, gitCommit.GitTree.Hash, err) - } else if len(filesChanged) > 0 && gitCommit.Commit.Change == nil { + } else if len(changedFiles) > 0 && gitCommit.Commit.Change == nil { return errors.New("files changes but commit is not a change commit") } - pathsChanged := make([]string, len(filesChanged)) - for i := range filesChanged { - pathsChanged[i] = filesChanged[i].path + pathsChanged := make([]string, len(changedFiles)) + for i := range changedFiles { + pathsChanged[i] = changedFiles[i].Path } commitType, err := gitCommit.Commit.Type() @@ -418,14 +417,14 @@ func (r *Repo) verifyCommit(branch plumbing.ReferenceName, gitCommit GitCommit, } // ensure the hash is what it's expected to be - commitHash := gitCommit.Interface.GetHash() - expectedCommitHash, err := gitCommit.Interface.Hash(parentTree, gitCommit.GitTree) + storedCommitHash := gitCommit.Interface.StoredHash() + expectedCommitHash, err := gitCommit.Interface.ExpectedHash(changedFiles) if err != nil { return fmt.Errorf("calculating expected commit hash: %w", err) - } else if !bytes.Equal(commitHash, expectedCommitHash) { + } else if !bytes.Equal(storedCommitHash, expectedCommitHash) { return fmt.Errorf("unexpected hash in commit body, is %s but should be %s", - base64.StdEncoding.EncodeToString(expectedCommitHash), - base64.StdEncoding.EncodeToString(commitHash)) + base64.StdEncoding.EncodeToString(storedCommitHash), + base64.StdEncoding.EncodeToString(expectedCommitHash)) } // verify all credentials @@ -485,6 +484,13 @@ func (r *Repo) changeRangeInfo(commits []GitCommit) (changeRangeInfo, error) { lastChangeCommit := info.changeCommits[len(info.changeCommits)-1] info.msg = lastChangeCommit.Commit.Change.Message info.endTree = lastChangeCommit.GitTree - info.changeHash = genChangeHash(nil, info.msg, info.startTree, info.endTree) + + changedFiles, err := ChangedFilesBetweenTrees(info.startTree, info.endTree) + if err != nil { + return changeRangeInfo{}, fmt.Errorf("calculating diff of commit trees %q and %q: %w", + info.startTree.Hash, info.endTree.Hash, err) + } + + info.changeHash = genChangeHash(nil, info.msg, changedFiles) return info, nil } diff --git a/commit_change.go b/commit_change.go index 5afa89f..6424a35 100644 --- a/commit_change.go +++ b/commit_change.go @@ -2,14 +2,15 @@ package dehub import ( "bytes" - "dehub.dev/src/dehub.git/fs" - "dehub.dev/src/dehub.git/sigcred" - "dehub.dev/src/dehub.git/yamlutil" "errors" "fmt" "sort" "strings" + "dehub.dev/src/dehub.git/fs" + "dehub.dev/src/dehub.git/sigcred" + "dehub.dev/src/dehub.git/yamlutil" + "gopkg.in/src-d/go-git.v4/plumbing" "gopkg.in/src-d/go-git.v4/plumbing/object" ) @@ -38,8 +39,13 @@ func (r *Repo) NewCommitChange(msg string) (Commit, error) { return Commit{}, err } + changedFiles, err := ChangedFilesBetweenTrees(headTree, stagedTree) + if err != nil { + return Commit{}, fmt.Errorf("calculating diff between HEAD and staged changes: %w", err) + } + cc := CommitChange{Message: msg} - if cc.ChangeHash, err = cc.Hash(headTree, stagedTree); err != nil { + if cc.ChangeHash, err = cc.ExpectedHash(changedFiles); err != nil { return Commit{}, err } @@ -53,13 +59,13 @@ func (cc CommitChange) MessageHead(CommitCommon) (string, error) { return abbrevCommitMessage(cc.Message), nil } -// Hash implements the method for the CommitInterface interface. -func (cc CommitChange) Hash(parent, this *object.Tree) ([]byte, error) { - return genChangeHash(nil, cc.Message, parent, this), nil +// ExpectedHash implements the method for the CommitInterface interface. +func (cc CommitChange) ExpectedHash(changedFiles []ChangedFile) ([]byte, error) { + return genChangeHash(nil, cc.Message, changedFiles), nil } -// GetHash implements the method for the CommitInterface interface. -func (cc CommitChange) GetHash() []byte { +// StoredHash implements the method for the CommitInterface interface. +func (cc CommitChange) StoredHash() []byte { return cc.ChangeHash } @@ -96,17 +102,23 @@ func (r *Repo) CombineCommitChanges(commits []GitCommit, onto plumbing.Reference if err != nil { return GitCommit{}, fmt.Errorf("resolving revision %q: %w", onto, err) } - ontoTree := ontoCommit.GitTree - ontoEndChangeHash := genChangeHash(nil, info.msg, ontoTree, info.endTree) + + ontoEndChangedFiles, err := ChangedFilesBetweenTrees(ontoCommit.GitTree, info.endTree) + if err != nil { + return GitCommit{}, fmt.Errorf("calculating file changes between %q and %q: %w", + ontoCommit.GitCommit.Hash, commits[len(commits)-1].GitCommit.Hash, err) + } + + ontoEndChangeHash := genChangeHash(nil, info.msg, ontoEndChangedFiles) if !bytes.Equal(ontoEndChangeHash, info.changeHash) { // TODO figure out what files to show as being the "problem files" in // the error message - return GitCommit{}, fmt.Errorf("rebasing onto %q would cause the change hash to change, aborting combine", onto) + return GitCommit{}, fmt.Errorf("combining onto %q would cause the change hash to change, aborting combine", onto.Short()) } var creds []sigcred.Credential for _, commit := range commits { - if bytes.Equal(commit.Interface.GetHash(), info.changeHash) { + if bytes.Equal(commit.Interface.StoredHash(), info.changeHash) { creds = append(creds, commit.Commit.Common.Credentials...) } } diff --git a/commit_change_test.go b/commit_change_test.go index c557d2d..94fbc1f 100644 --- a/commit_change_test.go +++ b/commit_change_test.go @@ -141,7 +141,7 @@ func TestCombineCommitChanges(t *testing.T) { fooCommit := h.assertCommitChange(true, "add foo file", rootSig) // now adding a credential commit from toot should work - credCommitObj, err := h.repo.NewCommitCredential(fooCommit.Interface.GetHash()) + credCommitObj, err := h.repo.NewCommitCredential(fooCommit.Interface.StoredHash()) if err != nil { t.Fatal(err) } diff --git a/commit_comment.go b/commit_comment.go index 72972cc..ad6a8e9 100644 --- a/commit_comment.go +++ b/commit_comment.go @@ -1,12 +1,11 @@ package dehub import ( + "errors" "fmt" "strings" "dehub.dev/src/dehub.git/yamlutil" - - "gopkg.in/src-d/go-git.v4/plumbing/object" ) // CommitComment describes the structure of a comment commit message. @@ -17,13 +16,13 @@ type CommitComment struct { var _ CommitInterface = CommitComment{} -// NewCommitComment constructs and returns a COmmit populated with a +// NewCommitComment constructs and returns a Commit populated with a // CommitComment encompassing the given message. The Credentials of the returned // Commit will _not_ be filled in. func (r *Repo) NewCommitComment(msg string) (Commit, error) { cc := CommitComment{Message: msg} var err error - if cc.MessageHash, err = cc.Hash(nil, nil); err != nil { + if cc.MessageHash, err = cc.ExpectedHash(nil); err != nil { return Commit{}, fmt.Errorf("calculating comment hash: %w", err) } return Commit{Comment: &cc}, nil @@ -36,12 +35,15 @@ func (cc CommitComment) MessageHead(common CommitCommon) (string, error) { return fmt.Sprintf("Comment by %s: %s", credIDs, msgAbbrev), nil } -// Hash implements the method for the CommitInterface. -func (cc CommitComment) Hash(_, _ *object.Tree) ([]byte, error) { +// ExpectedHash implements the method for the CommitInterface. +func (cc CommitComment) ExpectedHash(changes []ChangedFile) ([]byte, error) { + if len(changes) > 0 { + return nil, errors.New("CommitComment cannot have any changed files") + } return genCommentHash(nil, cc.Message), nil } -// GetHash implements the method for the CommitInterface. -func (cc CommitComment) GetHash() []byte { +// StoredHash implements the method for the CommitInterface. +func (cc CommitComment) StoredHash() []byte { return cc.MessageHash } diff --git a/commit_credential.go b/commit_credential.go index a2f639d..8364c41 100644 --- a/commit_credential.go +++ b/commit_credential.go @@ -2,12 +2,11 @@ package dehub import ( "encoding/base64" + "errors" "fmt" "strings" "dehub.dev/src/dehub.git/yamlutil" - - "gopkg.in/src-d/go-git.v4/plumbing/object" ) // CommitCredential describes the structure of a credential commit message. @@ -33,7 +32,7 @@ func (r *Repo) NewCommitCredential(hash []byte) (Commit, error) { }, nil } -// NewCommitCredentialFromChanges constructs and returns a Comit populated with +// NewCommitCredentialFromChanges constructs and returns a Commit populated with // a CommitCredential for all changes in the given range of GitCommits. The // message of the last change commit in the range is used when generating the // hash. @@ -68,12 +67,15 @@ func (cc CommitCredential) MessageHead(common CommitCommon) (string, error) { return fmt.Sprintf("Credential of hash %s by %s", hash64, credIDs), nil } -// Hash implements the method for the CommitInterface. -func (cc CommitCredential) Hash(_, _ *object.Tree) ([]byte, error) { +// ExpectedHash implements the method for the CommitInterface. +func (cc CommitCredential) ExpectedHash(changes []ChangedFile) ([]byte, error) { + if len(changes) > 0 { + return nil, errors.New("CommitCredential cannot have any changed files") + } return cc.CredentialedHash, nil } -// GetHash implements the method for the CommitInterface. -func (cc CommitCredential) GetHash() []byte { +// StoredHash implements the method for the CommitInterface. +func (cc CommitCredential) StoredHash() []byte { return cc.CredentialedHash } diff --git a/diff.go b/diff.go index d3aaa32..5bcfe4d 100644 --- a/diff.go +++ b/diff.go @@ -8,34 +8,38 @@ import ( "gopkg.in/src-d/go-git.v4/plumbing/object" ) -type fileChanged struct { - path string - fromMode, toMode filemode.FileMode - fromHash, toHash plumbing.Hash +// ChangedFile describes a single file which has been changed in some way +// between two object.Trees. If the From fields are empty then the file was +// created, if the To fields are empty then the file was deleted. +type ChangedFile struct { + Path string + FromMode, ToMode filemode.FileMode + FromHash, ToHash plumbing.Hash } -func calcDiff(from, to *object.Tree) ([]fileChanged, error) { - +// ChangedFilesBetweenTrees returns the ChangedFile objects which represent the +// difference between the two given trees. +func ChangedFilesBetweenTrees(from, to *object.Tree) ([]ChangedFile, error) { changes, err := object.DiffTree(from, to) if err != nil { return nil, fmt.Errorf("could not calculate tree diff: %w", err) } - filesChanged := make([]fileChanged, len(changes)) + changedFiles := make([]ChangedFile, len(changes)) for i, change := range changes { if from := change.From; from.Name != "" { - filesChanged[i].path = from.Name - filesChanged[i].fromMode = from.TreeEntry.Mode - filesChanged[i].fromHash = from.TreeEntry.Hash + changedFiles[i].Path = from.Name + changedFiles[i].FromMode = from.TreeEntry.Mode + changedFiles[i].FromHash = from.TreeEntry.Hash } if to := change.To; to.Name != "" { - if exPath := filesChanged[i].path; exPath != "" && exPath != to.Name { - panic(fmt.Sprintf("DiffTree entry changed path from %q to %q", exPath, to.Name)) + if exPath := changedFiles[i].Path; exPath != "" && exPath != to.Name { + panic(fmt.Sprintf("unexpected changed path from %q to %q", exPath, to.Name)) } - filesChanged[i].path = to.Name - filesChanged[i].toMode = to.TreeEntry.Mode - filesChanged[i].toHash = to.TreeEntry.Hash + changedFiles[i].Path = to.Name + changedFiles[i].ToMode = to.TreeEntry.Mode + changedFiles[i].ToHash = to.TreeEntry.Hash } } - return filesChanged, nil + return changedFiles, nil } diff --git a/hash.go b/hash.go index d969242..525ad20 100644 --- a/hash.go +++ b/hash.go @@ -6,8 +6,6 @@ import ( "fmt" "hash" "sort" - - "gopkg.in/src-d/go-git.v4/plumbing/object" ) var ( @@ -15,7 +13,7 @@ var ( ) type hashHelper struct { - hash.Hash + hash hash.Hash varintBuf []byte } @@ -25,49 +23,43 @@ func newHashHelper(h hash.Hash) *hashHelper { h = defaultHashHelperAlgo() } s := &hashHelper{ - Hash: h, + hash: h, varintBuf: make([]byte, binary.MaxVarintLen64), } return s } func (s *hashHelper) sum(prefix []byte) []byte { - out := make([]byte, len(prefix), len(prefix)+s.Hash.Size()) + out := make([]byte, len(prefix), len(prefix)+s.hash.Size()) copy(out, prefix) - return s.Hash.Sum(out) + return s.hash.Sum(out) } func (s *hashHelper) writeUint(i uint64) { n := binary.PutUvarint(s.varintBuf, i) - if _, err := s.Write(s.varintBuf[:n]); err != nil { - panic(fmt.Sprintf("error writing %x to sha256 sum: %v", s.varintBuf[:n], err)) + if _, err := s.hash.Write(s.varintBuf[:n]); err != nil { + panic(fmt.Sprintf("error writing %x to %T: %v", s.varintBuf[:n], s.hash, err)) } } func (s *hashHelper) writeStr(str string) { s.writeUint(uint64(len(str))) - s.Write([]byte(str)) + s.hash.Write([]byte(str)) } -func (s *hashHelper) writeTreeDiff(from, to *object.Tree) { - filesChanged, err := calcDiff(from, to) - if err != nil { - panic(err.Error()) - } - - sort.Slice(filesChanged, func(i, j int) bool { - return filesChanged[i].path < filesChanged[j].path +func (s *hashHelper) writeChangedFiles(changedFiles []ChangedFile) { + sort.Slice(changedFiles, func(i, j int) bool { + return changedFiles[i].Path < changedFiles[j].Path }) - s.writeUint(uint64(len(filesChanged))) - for _, fileChanged := range filesChanged { - s.writeStr(fileChanged.path) - s.Write(fileChanged.fromMode.Bytes()) - s.Write(fileChanged.fromHash[:]) - s.Write(fileChanged.toMode.Bytes()) - s.Write(fileChanged.toHash[:]) + s.writeUint(uint64(len(changedFiles))) + for _, fileChanged := range changedFiles { + s.writeStr(fileChanged.Path) + s.hash.Write(fileChanged.FromMode.Bytes()) + s.hash.Write(fileChanged.FromHash[:]) + s.hash.Write(fileChanged.ToMode.Bytes()) + s.hash.Write(fileChanged.ToHash[:]) } - } var ( @@ -76,10 +68,10 @@ var ( ) // if h is nil it then defaultHashHelperAlgo will be used -func genChangeHash(h hash.Hash, msg string, from, to *object.Tree) []byte { +func genChangeHash(h hash.Hash, msg string, changedFiles []ChangedFile) []byte { s := newHashHelper(h) s.writeStr(msg) - s.writeTreeDiff(from, to) + s.writeChangedFiles(changedFiles) return s.sum(changeHashVersion) } diff --git a/hash_test.go b/hash_test.go new file mode 100644 index 0000000..c3b6306 --- /dev/null +++ b/hash_test.go @@ -0,0 +1,237 @@ +package dehub + +import ( + "bytes" + "encoding/binary" + "hash" + "testing" + + "gopkg.in/src-d/go-git.v4/plumbing" + "gopkg.in/src-d/go-git.v4/plumbing/filemode" +) + +type testHash struct { + bytes.Buffer +} + +var _ hash.Hash = new(testHash) + +func (th *testHash) Sum(b []byte) []byte { + return append(b, th.Buffer.Bytes()...) +} + +func (th *testHash) Size() int { + return th.Buffer.Len() +} + +func (th *testHash) BlockSize() int { + return 1 +} + +func (th *testHash) assertContents(t *testing.T, parts [][]byte) { + b := th.Buffer.Bytes() + for _, part := range parts { + if len(part) > len(b) || !bytes.Equal(part, b[:len(part)]) { + t.Fatalf("expected %q but only found %q", part, b) + } + b = b[len(part):] + } + if len(b) != 0 { + t.Fatalf("unexpected extra bytes written to testHash: %q", b) + } +} + +func uvarint(i uint64) []byte { + buf := make([]byte, binary.MaxVarintLen64) + n := binary.PutUvarint(buf, i) + return buf[:n] +} + +func TestGenCommentHash(t *testing.T) { + type test struct { + descr string + comment string + exp [][]byte + } + + tests := []test{ + { + descr: "empty comment", + comment: "", + exp: [][]byte{uvarint(0)}, + }, + { + descr: "normal comment", + comment: "this is a normal comment", + exp: [][]byte{uvarint(24), []byte("this is a normal comment")}, + }, + { + descr: "comment with unicode", + comment: "sick comment ⚡", + exp: [][]byte{uvarint(16), []byte("sick comment ⚡")}, + }, + } + + for _, test := range tests { + t.Run(test.descr, func(t *testing.T) { + th := new(testHash) + genCommentHash(th, test.comment) + th.assertContents(t, test.exp) + }) + } +} + +func TestGenChangeHash(t *testing.T) { + type test struct { + descr string + msg string + changedFiles []ChangedFile + exp [][]byte + } + + hash := func(i byte) plumbing.Hash { + var h plumbing.Hash + h[0] = i + return h + } + hashB := func(i byte) []byte { + h := hash(i) + return h[:] + } + + tests := []test{ + { + descr: "empty", + msg: "", + changedFiles: nil, + exp: [][]byte{uvarint(0), uvarint(0)}, + }, + { + descr: "empty changes", + msg: "some msg", + changedFiles: nil, + exp: [][]byte{uvarint(8), []byte("some msg"), uvarint(0)}, + }, + { + descr: "empty msg", + msg: "", + changedFiles: []ChangedFile{{ + Path: "foo", + ToMode: filemode.Regular, ToHash: hash(1), + }}, + exp: [][]byte{uvarint(0), uvarint(1), + uvarint(3), []byte("foo"), + filemode.Empty.Bytes(), hashB(0), + filemode.Regular.Bytes(), hashB(1)}, + }, + { + descr: "files added", + msg: "a", + changedFiles: []ChangedFile{ + { + Path: "foo", + ToMode: filemode.Regular, ToHash: hash(1), + }, + { + Path: "somedir/bar", + ToMode: filemode.Executable, ToHash: hash(2), + }, + }, + exp: [][]byte{uvarint(1), []byte("a"), uvarint(2), + uvarint(3), []byte("foo"), + filemode.Empty.Bytes(), hashB(0), + filemode.Regular.Bytes(), hashB(1), + uvarint(11), []byte("somedir/bar"), + filemode.Empty.Bytes(), hashB(0), + filemode.Executable.Bytes(), hashB(2), + }, + }, + { + descr: "files added (unordered)", + msg: "a", + changedFiles: []ChangedFile{ + { + Path: "somedir/bar", + ToMode: filemode.Executable, ToHash: hash(2), + }, + { + Path: "foo", + ToMode: filemode.Regular, ToHash: hash(1), + }, + }, + exp: [][]byte{uvarint(1), []byte("a"), uvarint(2), + uvarint(3), []byte("foo"), + filemode.Empty.Bytes(), hashB(0), + filemode.Regular.Bytes(), hashB(1), + uvarint(11), []byte("somedir/bar"), + filemode.Empty.Bytes(), hashB(0), + filemode.Executable.Bytes(), hashB(2), + }, + }, + { + descr: "file modified", + msg: "a", + changedFiles: []ChangedFile{{ + Path: "foo", + FromMode: filemode.Regular, FromHash: hash(1), + ToMode: filemode.Executable, ToHash: hash(2), + }}, + exp: [][]byte{uvarint(1), []byte("a"), uvarint(1), + uvarint(3), []byte("foo"), + filemode.Regular.Bytes(), hashB(1), + filemode.Executable.Bytes(), hashB(2), + }, + }, + { + descr: "file removed", + msg: "a", + changedFiles: []ChangedFile{{ + Path: "foo", + FromMode: filemode.Regular, FromHash: hash(1), + }}, + exp: [][]byte{uvarint(1), []byte("a"), uvarint(1), + uvarint(3), []byte("foo"), + filemode.Regular.Bytes(), hashB(1), + filemode.Empty.Bytes(), hashB(0), + }, + }, + { + descr: "files added, modified, and removed", + msg: "aaa", + changedFiles: []ChangedFile{ + { + Path: "foo", + ToMode: filemode.Regular, ToHash: hash(1), + }, + { + Path: "bar", + FromMode: filemode.Regular, FromHash: hash(2), + ToMode: filemode.Regular, ToHash: hash(3), + }, + { + Path: "baz", + FromMode: filemode.Executable, FromHash: hash(4), + }, + }, + exp: [][]byte{uvarint(3), []byte("aaa"), uvarint(3), + uvarint(3), []byte("bar"), + filemode.Regular.Bytes(), hashB(2), + filemode.Regular.Bytes(), hashB(3), + uvarint(3), []byte("baz"), + filemode.Executable.Bytes(), hashB(4), + filemode.Empty.Bytes(), hashB(0), + uvarint(3), []byte("foo"), + filemode.Empty.Bytes(), hashB(0), + filemode.Regular.Bytes(), hashB(1), + }, + }, + } + + for _, test := range tests { + t.Run(test.descr, func(t *testing.T) { + th := new(testHash) + genChangeHash(th, test.msg, test.changedFiles) + th.assertContents(t, test.exp) + }) + } +}