Merge pull request #404 from fluxcd/commit-string-fmt

git: change Commit#String format

git/git.go

@@ -26,8 +26,36 @@ import (
 	"github.com/ProtonMail/go-crypto/openpgp"
 )
 
+const (
+	// HashTypeSHA1 is the SHA1 hash algorithm.
+	HashTypeSHA1 = "sha1"
+	// HashTypeUnknown is an unknown hash algorithm.
+	HashTypeUnknown = "<unknown>"
+)
+
+// Hash is the (non-truncated) SHA-1 or SHA-256 hash of a Git commit.
 type Hash []byte
 
+// Algorithm returns the algorithm of the hash based on its length.
+// This is heuristic, and may not be accurate for truncated user constructed
+// hashes. The library itself does not produce truncated hashes.
+func (h Hash) Algorithm() string {
+	switch len(h) {
+	case 40:
+		return HashTypeSHA1
+	default:
+		return HashTypeUnknown
+	}
+}
+
+// Digest returns a digest of the commit, in the format of "<algorithm>:<hash>".
+func (h Hash) Digest() string {
+	if len(h) == 0 {
+		return ""
+	}
+	return fmt.Sprintf("%s:%s", h.Algorithm(), h)
+}
+
 // String returns the Hash as a string.
 func (h Hash) String() string {
 	return string(h)
@@ -43,7 +71,7 @@ type Signature struct {
 
 // Commit contains all possible information about a Git commit.
 type Commit struct {
-	// Hash is the SHA1 hash of the commit.
+	// Hash is the hash of the commit.
 	Hash Hash
 	// Reference is the original reference of the commit, for example:
 	// 'refs/tags/foo'.
@@ -57,7 +85,7 @@ type Commit struct {
 	Signature string
 	// Encoded is the encoded commit, without any signature.
 	Encoded []byte
-	// Message is the commit message, contains arbitrary text.
+	// Message is the commit message, containing arbitrary text.
 	Message string
 }
 
@@ -67,9 +95,9 @@ type Commit struct {
 // for a "tag-1" tag.
 func (c *Commit) String() string {
 	if short := strings.SplitAfterN(c.Reference, "/", 3); len(short) == 3 {
-		return fmt.Sprintf("%s/%s", short[2], c.Hash)
+		return fmt.Sprintf("%s@%s", short[2], c.Hash.Digest())
 	}
-	return fmt.Sprintf("HEAD/%s", c.Hash)
+	return c.Hash.Digest()
 }
 
 // Verify the Signature of the commit with the given key rings.
@@ -121,8 +149,8 @@ var (
 )
 
 // IsConcreteCommit returns if a given commit is a concrete commit. Concrete
-// commits have most of commit metadata and commit content. In contrast, a
-// partial commit may only have some metadata and no commit content.
+// commits have most of the commit metadata and content. In contrast, a partial
+// commit may only have some metadata and no commit content.
 func IsConcreteCommit(c Commit) bool {
 	if c.Hash != nil && c.Encoded != nil {
 		return true

git/git_test.go

@@ -119,6 +119,83 @@ Bg2WzDuLKvQBi9tFSwnUbQoFFlOeiGW8G/bdkoJDWeS1oYgSD3nkmvXvrVESCrbT
 `
 )
 
+func TestHash_Algorithm(t *testing.T) {
+	tests := []struct {
+		name string
+		hash Hash
+		want string
+	}{
+		{
+			name: "SHA-1",
+			hash: Hash("5394cb7f48332b2de7c17dd8b8384bbc84b7e738"),
+			want: HashTypeSHA1,
+		},
+		{
+			name: "SHA-256",
+			hash: Hash("6ee9a7ade2ca791bc1bf9d133ef6ddaa9097cf521e6a19be92dbcc3f2e82f6d8"),
+			want: HashTypeUnknown,
+		},
+		{
+			name: "MD5",
+			hash: Hash("dba535cd50b291777a055338572e4a4b"),
+			want: HashTypeUnknown,
+		},
+		{
+			name: "Empty",
+			hash: Hash(""),
+			want: HashTypeUnknown,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			g.Expect(tt.hash.Algorithm()).To(Equal(tt.want))
+		})
+	}
+}
+
+func TestHash_Digest(t *testing.T) {
+	tests := []struct {
+		name string
+		hash Hash
+		want string
+	}{
+		{
+			name: "With a SHA-1 hash",
+			hash: Hash("5394cb7f48332b2de7c17dd8b8384bbc84b7e738"),
+			want: "sha1:5394cb7f48332b2de7c17dd8b8384bbc84b7e738",
+		},
+		{
+			name: "With an unknown (MD5) hash",
+			hash: Hash("dba535cd50b291777a055338572e4a4b"),
+			want: "<unknown>:dba535cd50b291777a055338572e4a4b",
+		},
+		{
+			name: "With an unknown (SHA-256) hash",
+			hash: Hash("6ee9a7ade2ca791bc1bf9d133ef6ddaa9097cf521e6a19be92dbcc3f2e82f6d8"),
+			want: "<unknown>:6ee9a7ade2ca791bc1bf9d133ef6ddaa9097cf521e6a19be92dbcc3f2e82f6d8",
+		},
+		{
+			name: "With a nil hash",
+			hash: nil,
+			want: "",
+		},
+		{
+			name: "With an empty hash",
+			hash: Hash(""),
+			want: "",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			g.Expect(tt.hash.Digest()).To(Equal(tt.want))
+		})
+	}
+}
+
 func TestCommit_String(t *testing.T) {
 	tests := []struct {
 		name   string
@@ -128,25 +205,25 @@ func TestCommit_String(t *testing.T) {
 		{
 			name: "Reference and commit",
 			commit: &Commit{
-				Hash:      []byte("commit"),
+				Hash:      []byte("5394cb7f48332b2de7c17dd8b8384bbc84b7e738"),
 				Reference: "refs/heads/main",
 			},
-			want: "main/commit",
+			want: "main@sha1:5394cb7f48332b2de7c17dd8b8384bbc84b7e738",
 		},
 		{
 			name: "Reference with slash and commit",
 			commit: &Commit{
-				Hash:      []byte("commit"),
+				Hash:      []byte("5394cb7f48332b2de7c17dd8b8384bbc84b7e738"),
 				Reference: "refs/heads/feature/branch",
 			},
-			want: "feature/branch/commit",
+			want: "feature/branch@sha1:5394cb7f48332b2de7c17dd8b8384bbc84b7e738",
 		},
 		{
-			name: "No reference",
+			name: "No name reference",
 			commit: &Commit{
-				Hash: []byte("commit"),
+				Hash: []byte("5394cb7f48332b2de7c17dd8b8384bbc84b7e738"),
 			},
-			want: "HEAD/commit",
+			want: "sha1:5394cb7f48332b2de7c17dd8b8384bbc84b7e738",
 		},
 	}
 	for _, tt := range tests {

git/gogit/clone.go

@@ -49,25 +49,14 @@ func (g *Client) cloneBranch(ctx context.Context, url, branch string, opts repos
 
 	ref := plumbing.NewBranchReferenceName(branch)
 	// check if previous revision has changed before attempting to clone
-	if opts.LastObservedCommit != "" {
+	if lastObserved := git.TransformRevision(opts.LastObservedCommit); lastObserved != "" {
 		head, err := getRemoteHEAD(ctx, url, ref, g.authOpts, authMethod)
 		if err != nil {
 			return nil, err
 		}
-
-		if head != "" && head == opts.LastObservedCommit {
-			// Construct a non-concrete commit with the existing information.
-			// Split the revision and take the last part as the hash.
-			// Example revision: main/43d7eb9c49cdd49b2494efd481aea1166fc22b67
-			var hash git.Hash
-			ss := strings.Split(head, "/")
-			if len(ss) > 1 {
-				hash = git.Hash(ss[len(ss)-1])
-			} else {
-				hash = git.Hash(ss[0])
-			}
+		if head != "" && head == lastObserved {
 			c := &git.Commit{
-				Hash:      hash,
+				Hash:      git.ExtractHashFromRevision(head),
 				Reference: plumbing.NewBranchReferenceName(branch).String(),
 			}
 			return c, nil
@@ -146,18 +135,8 @@ func (g *Client) cloneTag(ctx context.Context, url, tag string, opts repository.
 		}
 
 		if head != "" && head == opts.LastObservedCommit {
-			// Construct a non-concrete commit with the existing information.
-			// Split the revision and take the last part as the hash.
-			// Example revision: 6.1.4/bf09377bfd5d3bcac1e895fa8ce52dc76695c060
-			var hash git.Hash
-			ss := strings.Split(head, "/")
-			if len(ss) > 1 {
-				hash = git.Hash(ss[len(ss)-1])
-			} else {
-				hash = git.Hash(ss[0])
-			}
 			c := &git.Commit{
-				Hash:      hash,
+				Hash:      git.ExtractHashFromRevision(head),
 				Reference: ref.String(),
 			}
 			return c, nil
@@ -406,10 +385,9 @@ func getRemoteHEAD(ctx context.Context, url string, ref plumbing.ReferenceName,
 func filterRefs(refs []*plumbing.Reference, currentRef plumbing.ReferenceName) string {
 	for _, ref := range refs {
 		if ref.Name().String() == currentRef.String() {
-			return fmt.Sprintf("%s/%s", currentRef.Short(), ref.Hash().String())
+			return fmt.Sprintf("%s@%s", currentRef.Short(), git.Hash(ref.Hash().String()).Digest())
 		}
 	}
-
 	return ""
 }
 

git/gogit/clone_test.go

@@ -96,6 +96,14 @@ func TestClone_cloneBranch(t *testing.T) {
 			name:                   "skip clone if LastRevision hasn't changed",
 			branch:                 "master",
 			filesCreated:           map[string]string{"branch": "init"},
+			lastRevision:           fmt.Sprintf("master@%s", git.Hash(firstCommit.String()).Digest()),
+			expectedCommit:         firstCommit.String(),
+			expectedConcreteCommit: false,
+		},
+		{
+			name:                   "skip clone if LastRevision hasn't changed (legacy)",
+			branch:                 "master",
+			filesCreated:           map[string]string{"branch": "init"},
 			lastRevision:           fmt.Sprintf("master/%s", firstCommit.String()),
 			expectedCommit:         firstCommit.String(),
 			expectedConcreteCommit: false,
@@ -104,6 +112,14 @@ func TestClone_cloneBranch(t *testing.T) {
 			name:                   "Other branch - revision has changed",
 			branch:                 "test",
 			filesCreated:           map[string]string{"branch": "second"},
+			lastRevision:           fmt.Sprintf("master@%s", git.Hash(firstCommit.String()).Digest()),
+			expectedCommit:         secondCommit.String(),
+			expectedConcreteCommit: true,
+		},
+		{
+			name:                   "Other branch - revision has changed (legacy)",
+			branch:                 "test",
+			filesCreated:           map[string]string{"branch": "second"},
 			lastRevision:           fmt.Sprintf("master/%s", firstCommit.String()),
 			expectedCommit:         secondCommit.String(),
 			expectedConcreteCommit: true,
@@ -157,7 +173,7 @@ func TestClone_cloneBranch(t *testing.T) {
 			}
 
 			g.Expect(err).ToNot(HaveOccurred())
-			g.Expect(cc.String()).To(Equal(tt.branch + "/" + tt.expectedCommit))
+			g.Expect(cc.String()).To(Equal(tt.branch + "@" + git.HashTypeSHA1 + ":" + tt.expectedCommit))
 			g.Expect(git.IsConcreteCommit(*cc)).To(Equal(tt.expectedConcreteCommit))
 
 			if tt.expectedConcreteCommit {
@@ -258,7 +274,7 @@ func TestClone_cloneTag(t *testing.T) {
 			// If last revision is provided, configure it.
 			if tt.lastRevTag != "" {
 				lc := tagCommits[tt.lastRevTag]
-				opts.LastObservedCommit = fmt.Sprintf("%s/%s", tt.lastRevTag, lc)
+				opts.LastObservedCommit = fmt.Sprintf("%s@%s", tt.lastRevTag, git.Hash(lc).Digest())
 			}
 
 			cc, err := ggc.Clone(context.TODO(), path, opts)
@@ -274,7 +290,7 @@ func TestClone_cloneTag(t *testing.T) {
 			g.Expect(git.IsConcreteCommit(*cc)).To(Equal(tt.expectConcreteCommit))
 			targetTagHash := tagCommits[tt.checkoutTag]
 			g.Expect(err).ToNot(HaveOccurred())
-			g.Expect(cc.String()).To(Equal(tt.checkoutTag + "/" + targetTagHash))
+			g.Expect(cc.String()).To(Equal(tt.checkoutTag + "@" + git.HashTypeSHA1 + ":" + targetTagHash))
 
 			// Check file content only when there's an actual checkout.
 			if tt.lastRevTag != tt.checkoutTag {
@@ -314,14 +330,14 @@ func TestClone_cloneCommit(t *testing.T) {
 		{
 			name:         "Commit",
 			commit:       firstCommit.String(),
-			expectCommit: "HEAD/" + firstCommit.String(),
+			expectCommit: git.HashTypeSHA1 + ":" + firstCommit.String(),
 			expectFile:   "init",
 		},
 		{
 			name:         "Commit in specific branch",
 			commit:       secondCommit.String(),
 			branch:       "other-branch",
-			expectCommit: "other-branch/" + secondCommit.String(),
+			expectCommit: "other-branch@" + git.HashTypeSHA1 + ":" + secondCommit.String(),
 			expectFile:   "second",
 		},
 		{
@@ -470,7 +486,7 @@ func TestClone_cloneSemVer(t *testing.T) {
 			}
 
 			g.Expect(err).ToNot(HaveOccurred())
-			g.Expect(cc.String()).To(Equal(tt.expectTag + "/" + refs[tt.expectTag]))
+			g.Expect(cc.String()).To(Equal(tt.expectTag + "@" + git.HashTypeSHA1 + ":" + refs[tt.expectTag]))
 			g.Expect(filepath.Join(tmpDir, "tag")).To(BeARegularFile())
 			g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo(tt.expectTag))
 		})
@@ -970,7 +986,7 @@ func Test_getRemoteHEAD(t *testing.T) {
 	ref := plumbing.NewBranchReferenceName(git.DefaultBranch)
 	head, err := getRemoteHEAD(context.TODO(), path, ref, &git.AuthOptions{}, nil)
 	g.Expect(err).ToNot(HaveOccurred())
-	g.Expect(head).To(Equal(fmt.Sprintf("%s/%s", git.DefaultBranch, cc)))
+	g.Expect(head).To(Equal(fmt.Sprintf("%s@%s", git.DefaultBranch, git.Hash(cc.String()).Digest())))
 
 	cc, err = commitFile(repo, "test", "testing current head tag", time.Now())
 	g.Expect(err).ToNot(HaveOccurred())
@@ -980,7 +996,7 @@ func Test_getRemoteHEAD(t *testing.T) {
 	ref = plumbing.NewTagReferenceName("v0.1.0")
 	head, err = getRemoteHEAD(context.TODO(), path, ref, &git.AuthOptions{}, nil)
 	g.Expect(err).ToNot(HaveOccurred())
-	g.Expect(head).To(Equal(fmt.Sprintf("%s/%s", "v0.1.0", cc)))
+	g.Expect(head).To(Equal(fmt.Sprintf("%s@%s", "v0.1.0", git.Hash(cc.String()).Digest())))
 }
 
 func TestClone_CredentialsOverHttp(t *testing.T) {