From b45ba64805c9ff2f47aa9d775f684a1ee032ca59 Mon Sep 17 00:00:00 2001 From: Rob Figueiredo Date: Tue, 23 Sep 2014 15:51:15 -0400 Subject: [PATCH] logparser: fix - consecutive changed imports are now coalesced into updates. Add more tests. --- logparse_test.go | 122 ++++++++++++++++++++++++++++++++++++----------- logparser.go | 83 +++++++++++++++++++------------- 2 files changed, 144 insertions(+), 61 deletions(-) diff --git a/logparse_test.go b/logparse_test.go index d8e939f..f029d77 100644 --- a/logparse_test.go +++ b/logparse_test.go @@ -3,11 +3,12 @@ package main import ( "fmt" "reflect" + "sort" "strings" "testing" ) -const TEMPLATE = `25b4da1 change lgo4go to bogus revision +const templateWithContext = `25b4da1 change lgo4go to bogus revision diff --git a/REVISIONS b/REVISIONS index 82ef4f5..efc84fa 100644 --- a/REVISIONS @@ -21,84 +22,147 @@ index 82ef4f5..efc84fa 100644 github.com/codegangsta/martini/ 50f2d3f9d7eebef98b1a5dcf29dad72c66e918a7 ` +const templateNoContext = `25b4da1 change lgo4go to bogus revision +diff --git a/REVISIONS b/REVISIONS +index 82ef4f5..efc84fa 100644 +--- a/REVISIONS ++++ b/REVISIONS +@@ -1,6 +1,6 @@ +%s +` + var tests = []struct { name string - input string + diffs []string // one per "commit" book playbook }{ - {"update existing", fmt.Sprintf(TEMPLATE, ` + {"update existing", []string{` -code.google.com/p/log4go c3294304d93f +code.google.com/p/log4go 4794f7baff22 -`), playbook{library: []libraryAction{ +`}, playbook{library: []libraryAction{ {update, "code.google.com/p/log4go", "4794f7baff22"}, }}}, - {"update existing 2", fmt.Sprintf(TEMPLATE, ` -+code.google.com/p/log4go c3294304d93f --code.google.com/p/log4go 4794f7baff22 -`), playbook{library: []libraryAction{ - {update, "code.google.com/p/log4go", "c3294304d93f"}, + {"update existing (add first)", []string{` ++code.google.com/p/log4go 4794f7baff22 +-code.google.com/p/log4go c3294304d93f +`}, playbook{library: []libraryAction{ + {update, "code.google.com/p/log4go", "4794f7baff22"}, + }}}, + + {"update existing block", []string{` +-code.google.com/p/log4go c3294304d93f +-code.google.com/p/log4go2 4794f7baff22 ++code.google.com/p/log4go 4794f7baff22 ++code.google.com/p/log4go2 c3294304d93f +`}, playbook{library: []libraryAction{ + {update, "code.google.com/p/log4go", "4794f7baff22"}, + {update, "code.google.com/p/log4go2", "c3294304d93f"}, }}}, - {"add dep", fmt.Sprintf(TEMPLATE, ` + {"update existing block (add first)", []string{` +code.google.com/p/log4go 4794f7baff22 -`), playbook{library: []libraryAction{ ++code.google.com/p/log4go2 c3294304d93f +-code.google.com/p/log4go c3294304d93f +-code.google.com/p/log4go2 4794f7baff22 +`}, playbook{library: []libraryAction{ + {update, "code.google.com/p/log4go", "4794f7baff22"}, + {update, "code.google.com/p/log4go2", "c3294304d93f"}, + }}}, + + {"update existing block (intermixed)", []string{` ++code.google.com/p/log4go 4794f7baff22 +-code.google.com/p/log4go c3294304d93f ++code.google.com/p/log4go2 c3294304d93f +-code.google.com/p/log4go2 4794f7baff22 +`}, playbook{library: []libraryAction{ + {update, "code.google.com/p/log4go", "4794f7baff22"}, + {update, "code.google.com/p/log4go2", "c3294304d93f"}, + }}}, + + {"update existing block (intermixed 2)", []string{` ++code.google.com/p/log4go 4794f7baff22 +-code.google.com/p/log4go2 4794f7baff22 ++code.google.com/p/log4go2 c3294304d93f +-code.google.com/p/log4go c3294304d93f +`}, playbook{library: []libraryAction{ + {update, "code.google.com/p/log4go", "4794f7baff22"}, + {update, "code.google.com/p/log4go2", "c3294304d93f"}, + }}}, + + {"add dep", []string{` ++code.google.com/p/log4go 4794f7baff22 +`}, playbook{library: []libraryAction{ {add, "code.google.com/p/log4go", "4794f7baff22"}, }}}, - {"remove dep", fmt.Sprintf(TEMPLATE, ` + {"remove dep", []string{` -code.google.com/p/log4go 4794f7baff22 -`), playbook{library: []libraryAction{ +`}, playbook{library: []libraryAction{ {remove, "code.google.com/p/log4go", "4794f7baff22"}, }}}, - {"no change", "", playbook{}}, + {"no change", []string{""}, playbook{}}, - {"multiple commits", fmt.Sprintf(TEMPLATE, ` + {"multiple commits", []string{` +code.google.com/p/log4go 4794f7baff22 -`) + fmt.Sprintf(TEMPLATE, ` +`, ` +launchpad.net/gocheck 45 -`), playbook{library: []libraryAction{ +`}, playbook{library: []libraryAction{ {add, "code.google.com/p/log4go", "4794f7baff22"}, {add, "launchpad.net/gocheck", "45"}, }}}, - {"multiple commits on same import path", fmt.Sprintf(TEMPLATE, ` + {"multiple commits on same import path", []string{` -code.google.com/p/log4go 2 +code.google.com/p/log4go 3 -`) + fmt.Sprintf(TEMPLATE, ` +`, ` -code.google.com/p/log4go 1 +code.google.com/p/log4go 2 -`), playbook{library: []libraryAction{ +`}, playbook{library: []libraryAction{ {update, "code.google.com/p/log4go", "3"}, }}}, - {"exotic repo names", fmt.Sprintf(TEMPLATE, ` + {"exotic repo names", []string{` +code.google.com/p/go.tools 1 +github.com/shurcooL/Go_Package-Store 1 -`), playbook{library: []libraryAction{ +`}, playbook{library: []libraryAction{ {add, "code.google.com/p/go.tools", "1"}, {add, "github.com/shurcooL/Go_Package-Store", "1"}, }}}, - {"add cmd", fmt.Sprintf(TEMPLATE, ` + {"add cmd", []string{` +cmd code.google.com/p/go.tools/cmd/godoc -`), playbook{cmd: []cmdAction{ +`}, playbook{cmd: []cmdAction{ {true, "code.google.com/p/go.tools/cmd/godoc"}, }}}, - {"remove cmd", fmt.Sprintf(TEMPLATE, ` + {"remove cmd", []string{` -cmd code.google.com/p/go.tools/cmd/godoc -`), playbook{cmd: []cmdAction{ +`}, playbook{cmd: []cmdAction{ {false, "code.google.com/p/go.tools/cmd/godoc"}, }}}, } func TestLogParser(t *testing.T) { for _, test := range tests { - actual := buildPlaybook(readDiffLines(strings.NewReader(test.input))) - if !reflect.DeepEqual(actual, test.book) { - t.Errorf("%v: expected %d, got %d", test.name, test.book, actual) + for _, tmpl := range []string{templateWithContext, templateNoContext} { + var input string + for _, diff := range test.diffs { + input += fmt.Sprintf(tmpl, strings.TrimSpace(diff)) + } + actual := buildPlaybook(readDiffLines(strings.NewReader(input))) + // the library actions may be in any order, sort them. + sort.Sort(byLibImportPath(actual.library)) + if !reflect.DeepEqual(actual, test.book) { + t.Errorf("%v: expected %v, got %v", test.name, test.book, actual) + } } } } + +type byLibImportPath []libraryAction + +func (b byLibImportPath) Len() int { return len(b) } +func (b byLibImportPath) Swap(i, j int) { b[i], b[j] = b[j], b[i] } +func (b byLibImportPath) Less(i, j int) bool { return b[i].importPath < b[j].importPath } diff --git a/logparser.go b/logparser.go index 9c2884e..ba45c8f 100644 --- a/logparser.go +++ b/logparser.go @@ -78,54 +78,73 @@ func readDiffLines(reader io.Reader) []diff { return diffs } +func processDiffBlock(diffs []diff) []libraryAction { + // Assume: + // - An import path appears once or twice in a block. If twice, it was updated. + var importPathActions = make(map[string]libraryAction) + for _, this := range diffs { + // Calculate the new action for this import path. + // (Potentially this updates a previously recorded action from e.g. add to update) + if existing, ok := importPathActions[this.importPath]; ok { + if existing.action == update { + panic("unexpected: found import path " + this.importPath + " > 2 times in the diff") + } + importPathActions[this.importPath] = newUpdate(existing, this) + } else { + importPathActions[this.importPath] = newAddOrRemove(this) + } + } + + // Build all the library actions + var result []libraryAction + for _, action := range importPathActions { + result = append(result, action) + } + return result +} + func buildPlaybook(diffs []diff) playbook { // Convert diffs into actions. Since they may touch the same lines over // multiple commits, keep track of import paths that we've added commands for, // and only add the first. var ( book playbook + block []diff seenImportPaths = make(map[string]struct{}) ) - for i := 0; i < len(diffs); i++ { - var this = diffs[i] - if this == emptyLine { - continue - } - if this.revision == "cmd" { - book.cmd = append(book.cmd, cmdAction{this.added, this.importPath}) - continue - } - if _, ok := seenImportPaths[this.importPath]; ok { - continue - } - seenImportPaths[this.importPath] = struct{}{} - - // Is this an updated line pair (which we treat as a unit)? - var next = emptyLine - if i < len(diffs)-1 { - next = diffs[i+1] - } - if next != emptyLine && next.importPath == this.importPath { - if this.added == next.added { - panic("most unexpected") + for _, this := range append(diffs, emptyLine) { + switch { + case this == emptyLine: + // Consume the past block of diffs. Blocks of diffs are separated by emptyLine. + // Assume: + // - Blocks are processed in reverse chronological order. If we've seen an + // import path in a previous block, ignore it entirely. + if block == nil { + continue + } + for _, libAction := range processDiffBlock(block) { + if _, ok := seenImportPaths[libAction.importPath]; ok { + continue + } + seenImportPaths[libAction.importPath] = struct{}{} + book.library = append(book.library, libAction) } - book.library = append(book.library, newUpdate(this, next)) - i++ - continue + block = nil + case this.revision == "cmd": + book.cmd = append(book.cmd, cmdAction{this.added, this.importPath}) + default: + block = append(block, this) } - - // If not, record this as an action. - book.library = append(book.library, newAddOrRemove(this)) } return book } -func newUpdate(a, b diff) libraryAction { - var added = a +func newUpdate(a libraryAction, b diff) libraryAction { if b.added { - added = b + return newCommand(update, b) } - return newCommand(update, added) + a.action = update + return a } func newAddOrRemove(d diff) libraryAction {