Optimisation - reduce git note gets (#721)

* Add method to list revisions with notes

This method uses git notes list to get all notes for a given note ref.
We then turn the list into an array and take the second field from the
results (which corresponds to the object reference - the commit id in
our case). Finally, the result is placed in a map to make it easier
to do "if note is in" type queries later.

* Check if notes exist before requesting them

This is an optimisation for #714. We perform a single gitCommandExec to
get hashes for all commits with notes, place them into a map and use
them to query whether a commit has a note or not. This prevents
multiple calls to gitCommandExec just to see if there is a note
attached.

* Add extra check for error seen on ubuntu systems.

* Add context.

daemon/loop.go

@@ -176,6 +176,12 @@ func (d *Daemon) doSync(logger log.Logger) {
 		serviceIDs.Add(r.ServiceIDs(allResources))
 	}
 
+	notes, err := working.NoteRevList(ctx)
+	if err != nil {
+		logger.Log("err", errors.Wrap(err, "loading notes from repo"))
+		return
+	}
+
 	// Collect any events that come from notes attached to the commits
 	// we just synced. While we're doing this, keep track of what
 	// other things this sync includes e.g., releases and
@@ -187,6 +193,10 @@ func (d *Daemon) doSync(logger log.Logger) {
 
 		// Find notes in revisions.
 		for i := len(commits) - 1; i >= 0; i-- {
+			if ok := notes[commits[i].Revision]; !ok {
+				includes[history.NoneOfTheAbove] = true
+				continue
+			}
 			n, err := working.GetNote(ctx, commits[i].Revision)
 			if err != nil {
 				logger.Log("err", errors.Wrap(err, "loading notes from repo; possibly no notes"))

git/operations.go

@@ -120,6 +120,25 @@ func getNote(ctx context.Context, workingDir, notesRef, rev string) (*Note, erro
 	return &note, nil
 }
 
+// Get all revisions with a note (NB: DO NOT RELY ON THE ORDERING)
+// It appears to be ordered by ascending git object ref, not by time.
+// Return a map to make it easier to do "if in" type queries.
+func noteRevList(ctx context.Context, workingDir, notesRef string) (map[string]bool, error) {
+	out := &bytes.Buffer{}
+	if err := execGitCmd(ctx, workingDir, nil, out, "notes", "--ref", notesRef, "list"); err != nil {
+		return nil, err
+	}
+	noteList := splitList(out.String())
+	result := make(map[string]bool, len(noteList))
+	for _, l := range noteList {
+		split := strings.Fields(l)
+		if len(split) > 0 {
+			result[split[1]] = true // First field contains the object ref (commit id in our case)
+		}
+	}
+	return result, nil
+}
+
 // Get the commit hash for a reference
 func refRevision(ctx context.Context, path, ref string) (string, error) {
 	out := &bytes.Buffer{}
@@ -239,6 +258,8 @@ func findErrorMessage(output io.Reader) string {
 		switch {
 		case strings.HasPrefix(sc.Text(), "fatal: "):
 			return sc.Text()
+		case strings.HasPrefix(sc.Text(), "ERROR fatal: "): // Saw this error on ubuntu systems
+			return sc.Text()
 		case strings.HasPrefix(sc.Text(), "error:"):
 			return strings.Trim(sc.Text(), "error: ")
 		}

git/operations_test.go

@@ -2,13 +2,103 @@ package git
 
 import (
 	"context"
-	"github.com/weaveworks/flux/cluster/kubernetes/testfiles"
+	"fmt"
 	"io/ioutil"
 	"os/exec"
 	"path"
 	"testing"
+
+	"github.com/weaveworks/flux/cluster/kubernetes/testfiles"
+	"github.com/weaveworks/flux/job"
+	"github.com/weaveworks/flux/update"
+)
+
+const (
+	testNoteRef = "flux-sync"
+)
+
+var (
+	noteIdCounter = 1
 )
 
+func TestListNotes_2Notes(t *testing.T) {
+	newDir, cleanup := testfiles.TempDir(t)
+	defer cleanup()
+
+	err := createRepo(newDir, "another")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	idHEAD_1, err := testNote(newDir, "HEAD~1")
+	if err != nil {
+		t.Fatal(err)
+	}
+	idHEAD, err := testNote(newDir, "HEAD")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	notes, err := noteRevList(context.Background(), newDir, testNoteRef)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Now check that these commits actually have a note
+	if len(notes) != 2 {
+		t.Fatal("expected two notes")
+	}
+	for n := range notes {
+		note, err := getNote(context.Background(), newDir, testNoteRef, n)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if note == nil {
+			t.Fatal("note is nil")
+		}
+		if note.JobID != idHEAD_1 && note.JobID != idHEAD {
+			t.Fatal("Note id didn't match expected", note.JobID)
+		}
+	}
+}
+
+func TestListNotes_0Notes(t *testing.T) {
+	newDir, cleanup := testfiles.TempDir(t)
+	defer cleanup()
+
+	err := createRepo(newDir, "another")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	notes, err := noteRevList(context.Background(), newDir, testNoteRef)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if len(notes) != 0 {
+		t.Fatal("expected two notes")
+	}
+}
+
+func testNote(dir, rev string) (job.ID, error) {
+	id := job.ID(fmt.Sprintf("%v", noteIdCounter))
+	noteIdCounter += 1
+	err := addNote(context.Background(), dir, rev, testNoteRef, &Note{
+		id,
+		update.Spec{
+			update.Auto,
+			update.Cause{
+				"message",
+				"user",
+			},
+			update.Automated{},
+		},
+		update.Result{},
+	})
+	return id, err
+}
+
 func TestChangedFiles_SlashPath(t *testing.T) {
 	newDir, cleanup := testfiles.TempDir(t)
 	defer cleanup()
@@ -66,6 +156,9 @@ func createRepo(dir string, nestedDir string) error {
 	if err = execCommand("git", "-C", dir, "init"); err != nil {
 		return err
 	}
+	if err := config(context.Background(), dir, "operations_test_user", "example@example.com"); err != nil {
+		return err
+	}
 	if err := execCommand("mkdir", "-p", fullPath); err != nil {
 		return err
 	}
@@ -78,6 +171,9 @@ func createRepo(dir string, nestedDir string) error {
 	if err = execCommand("git", "-C", dir, "commit", "-m", "'Initial revision'"); err != nil {
 		return err
 	}
+	if err = execCommand("git", "-C", dir, "commit", "--allow-empty", "-m", "'Second revision'"); err != nil {
+		return err
+	}
 	return nil
 }
 

git/repo.go

@@ -239,3 +239,9 @@ func (c *Checkout) ChangedFiles(ctx context.Context, ref string) ([]string, erro
 	}
 	return list, err
 }
+
+func (c *Checkout) NoteRevList(ctx context.Context) (map[string]bool, error) {
+	c.Lock()
+	defer c.Unlock()
+	return noteRevList(ctx, c.Dir, c.SyncTag)
+}