From 6176b9ffbc7c43c5cdb79713dfd08524480f68dc Mon Sep 17 00:00:00 2001 From: mediocregopher <> Date: Wed, 13 May 2020 22:19:04 -0600 Subject: [PATCH] add change_description field to credential commits --- type: change description: |- add change_description field to credential commits This also involved changing how commit range change fingerprints are handled, so that the description can be more variable, and using that flexibility to allow the cli tool the ability to ask for the change description prior to creating a credential commit. Credential commit creation via the cli tool has been further improved in that it will check if it's been told to make a cred based on a specific credential commit, and if so just append the new credential and re-commit, speeding things up a bit for everyone. fingerprint: AGWXqPM1Lxf3kvV/V3w65jNiUQyrQfemqfCcImSWjHJl credentials: - type: pgp_signature pub_key_id: 95C46FA6A41148AC body: iQIzBAABAgAdFiEEJ6tQKp6olvZKJ0lwlcRvpqQRSKwFAl68xrgACgkQlcRvpqQRSKx6Ng//Qm/8dCqBe/8R7KgzW2naP352jegW5m9KoIMrVsm1kmgQfAGYs0easnks1+DadD79TEUacTrPYxDR50eScU58uybRUPPbJLXLjdoSbUYCl9bSdr6LVlZF9GVcMbH4iWhTN3YGQFHX7pEqwJ9Qzw/pbheCGPkS3KJFJZ3tC+TVM7QysdkGpxlKn8GiSIfN3d4QaHWFJW9FOc1K4mAFfmM/5QqU5j5OydlLeScJu+na7B8Lcmy4Jz+mdwIZCMG9h4qv1qD0/XcsY8Wv6cptMIBUZgg2SIyrl0KDau8pHX45PHlAaViCjzW2LjU2wMn+zKX6s3hSWje5sE95Z1a7l0ubytHGKUdlfTQDnkVYzs9hCqv3iAERChGl6E1qQWSNQMhhUcwVsZ7+Iw2M6ESrP/MvdsdO+N0t2oakbJnzUliGx8pQcY4DJPaosUadLqwKPMYiBOiSsfN2e+Vw2ZOoiHL2QUEd2VV1JfImZBl5oLFhzuLfR6h+eMkwcHjs+rtgMEh9TnA6qVzn7BzZ9p1ZFBHlw3Yrx22tG2Ehri4eMPa+5+vcuG7xlSlq4FOGvANi3E3lieeIiE61EztoX3iXeY4bdtaFlMEk6i+JU3jHdwXd006Kv0/5jd7n+OAba8aYjthNN2WuhTxj2/VSKn9iPjaCD3GJtkFQajf57v2nbK0QDx4= account: mediocregopher --- SPEC.md | 6 +++++- cmd/dehub/cmd_commit.go | 32 +++++++++++++++++++++++++------- cmd/dehub/tmp_file.go | 4 ++-- payload.go | 34 ++++++++++++++++++++++++++++------ payload_change.go | 15 ++++++++++----- payload_credential.go | 27 ++++++++++++++++++++++----- 6 files changed, 92 insertions(+), 26 deletions(-) diff --git a/SPEC.md b/SPEC.md index 9c3ffe1..b2ecdb1 100644 --- a/SPEC.md +++ b/SPEC.md @@ -174,10 +174,14 @@ credentials: [...] # This field is not required, but can be helpful in situations where the # fingerprint was generated based on multiple change payloads -fingerprint_commits: +commits: - commit hash - commit hash - commit hash + +# This field is not required, but can be helpful to clarify which description +# was used when generating a change fingerprint. +change_description: blah blah blah ``` ## Project Configuration diff --git a/cmd/dehub/cmd_commit.go b/cmd/dehub/cmd_commit.go index 862652a..707610e 100644 --- a/cmd/dehub/cmd_commit.go +++ b/cmd/dehub/cmd_commit.go @@ -136,25 +136,43 @@ func cmdCommit(ctx context.Context, cmd *dcmd.Cmd) { return nil, errors.New("credential commit cannot have staged changes") } - var credPayUn dehub.PayloadUnion + var commits []dehub.Commit if *rev != "" { commit, err := proj.GetCommitByRevision(plumbing.Revision(*rev)) if err != nil { return nil, fmt.Errorf("resolving revision %q: %w", *rev, err) } - - if credPayUn, err = proj.NewPayloadCredentialFromChanges([]dehub.Commit{commit}); err != nil { - return nil, fmt.Errorf("constructing credential commit: %w", err) - } + commits = []dehub.Commit{commit} } else { - commits, err := proj.GetCommitRangeByRevision( + var err error + commits, err = proj.GetCommitRangeByRevision( plumbing.Revision(*startRev), plumbing.Revision(*endRev), ) if err != nil { return nil, fmt.Errorf("resolving revisions %q to %q: %w", *startRev, *endRev, err) - } else if credPayUn, err = proj.NewPayloadCredentialFromChanges(commits); err != nil { + } + } + + var credPayUn dehub.PayloadUnion + if len(commits) == 0 { + return nil, errors.New("cannot create credential based on empty range of commits") + } else if len(commits) == 1 && commits[0].Payload.Credential != nil { + credPayUn = commits[0].Payload + } else { + lastDescr, err := dehub.LastChangeDescription(commits) + if err != nil { + return nil, fmt.Errorf("determining change description of commit(s): %w", err) + } + + descr, err := tmpFileMsg(defaultCommitFileMsgTpl, lastDescr) + if err != nil { + return nil, fmt.Errorf("collecting credential description from user: %w", err) + } + + credPayUn, err = proj.NewPayloadCredentialFromChanges(descr, commits) + if err != nil { return nil, fmt.Errorf("constructing credential commit: %w", err) } } diff --git a/cmd/dehub/tmp_file.go b/cmd/dehub/tmp_file.go index da251c7..59f54a6 100644 --- a/cmd/dehub/tmp_file.go +++ b/cmd/dehub/tmp_file.go @@ -14,8 +14,8 @@ import ( const defaultCommitFileMsgTpl = `%s -# Please enter the commit message for your commit. Lines starting -# with '#' will be ignored, and an empty message aborts the commit.` +# Please enter the description for your commit(s). Lines starting with '#' will +# be ignored, and an empty message aborts the commit.` func tmpFileMsg(tpl string, args ...interface{}) (string, error) { editor := os.Getenv("EDITOR") diff --git a/payload.go b/payload.go index 15d722f..e2bbc4e 100644 --- a/payload.go +++ b/payload.go @@ -490,12 +490,31 @@ func (proj *Project) verifyCommit( return nil } +// LastChangeDescription iterates over the given commits in reverse order and +// returns the first change description it comes across. A change description +// may come from a change payload or a credential payload which covers a set of +// changes. +// +// This function will return an error if no given commits contain a change +// description. +func LastChangeDescription(commits []Commit) (string, error) { + for i := range commits { + i = len(commits) - 1 - i + payUn := commits[i].Payload + if payUn.Change != nil { + return payUn.Change.Description, nil + } else if payUn.Credential != nil && payUn.Credential.ChangeDescription != "" { + return payUn.Credential.ChangeDescription, nil + } + } + return "", errors.New("no commits in range contain a change description") +} + type changeRangeInfo struct { changeCommits []Commit authors map[string]struct{} - msg string startTree, endTree *object.Tree - changeFingerprint []byte + changeDescription string } // changeRangeInfo returns various pieces of information about a range of @@ -523,20 +542,23 @@ func (proj *Project) changeRangeInfo(commits []Commit) (changeRangeInfo, error) if info.startTree, err = proj.parentTree(commits[0].Object); err != nil { return changeRangeInfo{}, fmt.Errorf("getting tree of parent of %q: %w", commits[0].Hash, err) + } else if info.changeDescription, err = LastChangeDescription(commits); err != nil { + return changeRangeInfo{}, err } lastChangeCommit := info.changeCommits[len(info.changeCommits)-1] - info.msg = lastChangeCommit.Payload.Change.Description info.endTree = lastChangeCommit.TreeObject + return info, nil +} +func (info changeRangeInfo) changeFingerprint(descr string) ([]byte, error) { changedFiles, err := ChangedFilesBetweenTrees(info.startTree, info.endTree) if err != nil { - return changeRangeInfo{}, fmt.Errorf("calculating diff of commit trees %q and %q: %w", + return nil, fmt.Errorf("calculating diff of commit trees %q and %q: %w", info.startTree.Hash, info.endTree.Hash, err) } - info.changeFingerprint = genChangeFingerprint(nil, info.msg, changedFiles) - return info, nil + return genChangeFingerprint(nil, descr, changedFiles), nil } // VerifyCanSetBranchHEADTo is used to verify that a branch's HEAD can be set to diff --git a/payload_change.go b/payload_change.go index c252e3d..d97bb4d 100644 --- a/payload_change.go +++ b/payload_change.go @@ -98,6 +98,11 @@ func (proj *Project) CombinePayloadChanges(commits []Commit, onto plumbing.Refer return Commit{}, err } + commitsFingerprint, err := info.changeFingerprint(info.changeDescription) + if err != nil { + return Commit{}, err + } + authors := make([]string, 0, len(info.authors)) for author := range info.authors { authors = append(authors, author) @@ -122,8 +127,8 @@ func (proj *Project) CombinePayloadChanges(commits []Commit, onto plumbing.Refer ontoCommit.Hash, commits[len(commits)-1].Hash, err) } - ontoEndChangeFingerprint := genChangeFingerprint(nil, info.msg, ontoEndChangedFiles) - if !bytes.Equal(ontoEndChangeFingerprint, info.changeFingerprint) { + ontoEndChangeFingerprint := genChangeFingerprint(nil, info.changeDescription, ontoEndChangedFiles) + if !bytes.Equal(ontoEndChangeFingerprint, commitsFingerprint) { // TODO figure out what files to show as being the "problem files" in // the error message return Commit{}, fmt.Errorf("combining onto %q would produce a different change fingerprint, aborting combine", onto.Short()) @@ -131,7 +136,7 @@ func (proj *Project) CombinePayloadChanges(commits []Commit, onto plumbing.Refer var creds []sigcred.CredentialUnion for _, commit := range commits { - if bytes.Equal(commit.Payload.Common.Fingerprint, info.changeFingerprint) { + if bytes.Equal(commit.Payload.Common.Fingerprint, commitsFingerprint) { creds = append(creds, commit.Payload.Common.Credentials...) } } @@ -143,10 +148,10 @@ func (proj *Project) CombinePayloadChanges(commits []Commit, onto plumbing.Refer payUn := PayloadUnion{ Change: &PayloadChange{ - Description: info.msg, + Description: info.changeDescription, }, Common: PayloadCommon{ - Fingerprint: info.changeFingerprint, + Fingerprint: commitsFingerprint, Credentials: creds, }, } diff --git a/payload_credential.go b/payload_credential.go index 9b114d0..ee3817f 100644 --- a/payload_credential.go +++ b/payload_credential.go @@ -10,6 +10,11 @@ type PayloadCredential struct { // It is only present for informational purposes, as commits don't not have // any bearing on the CredentialedHash itself. CommitHashes []string `yaml:"commits,omitempty"` + + // ChangeDescription represents the description which has been credentialed. + // This field is only relevant if the Credential in the payload is for a + // change set. + ChangeDescription string `yaml:"change_description"` } var _ Payload = PayloadCredential{} @@ -26,26 +31,38 @@ func (proj *Project) NewPayloadCredential(fingerprint []byte) (PayloadUnion, err // NewPayloadCredentialFromChanges constructs and returns a PayloadUnion // populated with a PayloadCredential. The fingerprint of the payload will be a -// change fingerprint encompassing all changes in the given range of Commits. -// The description of the last change payload in the range is used when -// generating the fingerprint. -func (proj *Project) NewPayloadCredentialFromChanges(commits []Commit) (PayloadUnion, error) { +// change fingerprint generated from the given description and all changes in +// the given range of Commits. +// +// If an empty description is given then the description of the last change +// payload in the range is used when generating the fingerprint. +func (proj *Project) NewPayloadCredentialFromChanges(descr string, commits []Commit) (PayloadUnion, error) { info, err := proj.changeRangeInfo(commits) if err != nil { return PayloadUnion{}, err } - payCred, err := proj.NewPayloadCredential(info.changeFingerprint) + if descr == "" { + descr = info.changeDescription + } + fingerprint, err := info.changeFingerprint(descr) if err != nil { return PayloadUnion{}, err } + payCred, err := proj.NewPayloadCredential(fingerprint) + if err != nil { + return PayloadUnion{}, err + } + + payCred.Credential.ChangeDescription = descr for _, commit := range info.changeCommits { payCred.Credential.CommitHashes = append( payCred.Credential.CommitHashes, commit.Hash.String(), ) } + return payCred, nil }