diff --git a/cmd/pullrequest/main.go b/cmd/pullrequest/main.go index eb51a98..df807c0 100644 --- a/cmd/pullrequest/main.go +++ b/cmd/pullrequest/main.go @@ -138,7 +138,12 @@ func readChange(event pullrequest.Event, git func(args ...string) ([]byte, error // commit landing on the base is not this pull request's change. span := event.Base + "..." + event.Head - out, err := git("diff", "--name-status", "-z", span) + // Rename detection is asked for rather than relied on. git turns it on by + // default, so leaving it out reads the same on almost every machine and + // differently on one that set diff.renames off, and the rule over a + // renamed experiment would then refuse a move as a removal for a reason + // living in somebody's git config. + out, err := git("diff", "--name-status", "--find-renames", "-z", span) if err != nil { return change, err } diff --git a/internal/pullrequest/pullrequest.go b/internal/pullrequest/pullrequest.go index 3b84381..2e1ffa2 100644 --- a/internal/pullrequest/pullrequest.go +++ b/internal/pullrequest/pullrequest.go @@ -200,6 +200,15 @@ type File struct { // record rule treats a removal as touching the experiment, for the reason // written at that property. Gone bool + + // From is where this file was moved from, and it is empty on every file + // that was not moved. git reports a rename as one entry carrying two + // paths, and the pairing between them is the whole of what the entry adds + // over a removal beside an addition. Dropping it costs the rule over a + // renamed experiment the second path its refusal has to name, and a + // refusal that names only where a record used to be sends the reader + // looking for where it went. + From string } // A Commit is one commit in the range, as much of it as any rule here reads. @@ -349,6 +358,7 @@ func Judge(change Change) Verdict { verdict.add(judgeCommits(change)) verdict.add(judgeExperiments(change)) verdict.add(judgeRecords(change)) + verdict.add(judgeRemovals(change)) verdict.add(judgeSize(change)) return verdict @@ -418,8 +428,8 @@ func judgeCommits(change Change) Verdict { // A change that removes an experiment's record along with its files satisfies // this rule, because the record is in the set of paths the change touched. That // a landed record may not be removed at all is a different rule about a -// different failure, and it is issue #69's rather than this one's. Nothing here -// should be read as permitting it. +// different failure, and it is record-already-landed-was-removed rather than +// this one. Nothing here should be read as permitting it. func judgeExperiments(change Change) Verdict { if !change.FilesRead { return Verdict{Skips: []Skip{{ diff --git a/internal/pullrequest/pullrequest_test.go b/internal/pullrequest/pullrequest_test.go index 4790a17..5b0fd9b 100644 --- a/internal/pullrequest/pullrequest_test.go +++ b/internal/pullrequest/pullrequest_test.go @@ -79,7 +79,7 @@ func TestJudge(t *testing.T) { // to the rule they are about, and they are returned into this table rather than // run from a second harness. func judgeCases() []judgeCase { - return append(recordJudgeCases(), []judgeCase{ + return append(append(recordJudgeCases(), removalJudgeCases()...), []judgeCase{ { name: "a change that names its issue and touches no experiment", change: clean(), @@ -290,6 +290,8 @@ func judgeCases() []judgeCase { ExperimentChangedWithoutItsRecord, AnswerAlreadyLandedWasRewritten, QuestionAlreadyAskedWasRewritten, + RecordAlreadyLandedWasRemoved, + ExperimentAlreadyLandedWasRenamed, ChangeIsLargerThanOneReading, }, }, @@ -311,6 +313,8 @@ func TestEveryPropertyHasACaseThatRefusesIt(t *testing.T) { ExperimentChangedWithoutItsRecord, AnswerAlreadyLandedWasRewritten, QuestionAlreadyAskedWasRewritten, + RecordAlreadyLandedWasRemoved, + ExperimentAlreadyLandedWasRenamed, } refused := make(map[string]bool) diff --git a/internal/pullrequest/read.go b/internal/pullrequest/read.go index 82ca0c4..5fdcee2 100644 --- a/internal/pullrequest/read.go +++ b/internal/pullrequest/read.go @@ -108,7 +108,10 @@ func ParseEvent(data []byte) (Event, error) { // A rename produces two paths for one entry, and both are returned. The old one // is gone at the head and the new one is not, which is what the record rule // needs: moving a file out of an experiment is a change to that experiment -// whichever end of the move a reader looks at. +// whichever end of the move a reader looks at. The new one also carries where +// it came from, because a removal beside an addition and a move are the same +// two paths otherwise, and the rule over a renamed experiment has to name both +// ends of the move. func ParseFiles(data []byte) ([]File, error) { fields := splitNul(data) var files []File @@ -131,7 +134,7 @@ func ParseFiles(data []byte) ([]File, error) { case 2: files = append(files, File{Path: fields[i+1], Gone: true}, - File{Path: fields[i+2], Gone: false}) + File{Path: fields[i+2], Gone: false, From: fields[i+1]}) } i += paths } diff --git a/internal/pullrequest/read_test.go b/internal/pullrequest/read_test.go index 9ec253c..253170b 100644 --- a/internal/pullrequest/read_test.go +++ b/internal/pullrequest/read_test.go @@ -60,7 +60,7 @@ func TestParseFiles(t *testing.T) { out: "R100\x00experiments/one/measure.go\x00experiments/two/measure.go\x00", want: []File{ {Path: "experiments/one/measure.go", Gone: true}, - {Path: "experiments/two/measure.go"}, + {Path: "experiments/two/measure.go", From: "experiments/one/measure.go"}, }, }, { diff --git a/internal/pullrequest/record.go b/internal/pullrequest/record.go index 100690f..404bc2f 100644 --- a/internal/pullrequest/record.go +++ b/internal/pullrequest/record.go @@ -76,7 +76,8 @@ type RecordChange struct { // After is the record at the head of the range, and AfterPresent says it // is still there. A record this change removes has no after. That a landed // record may not be removed at all is a different rule about a different - // failure, and it is issue #69's rather than this one's. + // failure, and it is record-already-landed-was-removed in removal.go + // rather than either of the two here. After []byte AfterPresent bool } diff --git a/internal/pullrequest/removal.go b/internal/pullrequest/removal.go new file mode 100644 index 0000000..30c4899 --- /dev/null +++ b/internal/pullrequest/removal.go @@ -0,0 +1,134 @@ +package pullrequest + +// The other half of the rule this board's central claim rests on. A record is +// not rewritten to hide what it said, which record.go holds, and it is not +// deleted, which is here. +// +// Deleting was the cheaper of the two and nothing refused it. A change that +// removes an EXPERIMENT.md, or the directory it sits in, passes every check +// that walks a tree, because the runner walks what is there and a record that +// is gone is not a record it can meet. So the rule held against editing and not +// against removing, and removing is the way to make an experiment stop having +// existed. +// +// WHY IT IS HERE AND NOT IN THE RUNNER, which is record.go's reason one step +// further on. Telling a removal from a record that was never there needs the +// version at the base of the range, which is history rather than a tree, and +// giving the runner a history reader would cost it the dependency surface +// record 0001 chose and the claim that it opens no connection leans on. This +// check already holds both ends of the range, so the comparison belongs beside +// the answer rule it completes. + +import "fmt" + +// RecordAlreadyLandedWasRemoved refuses a change that removes a record which +// was already on the branch the change lands on. +// +// The failure is ordinary rather than malicious, and that is what makes it +// likely. An experiment answered no, its prototype was removed under record +// 0004, and what is left looks like an empty directory somebody forgot to tidy. +// Tidying it takes the record with it, the tree is green afterwards, and the +// only thing that said the work happened at all is gone. +// +// What stays allowed is exactly what record 0004 already permits: the code +// goes, the record stays, and it gains the line naming the commit that removed +// the code. +const RecordAlreadyLandedWasRemoved = "record-already-landed-was-removed" + +// ExperimentAlreadyLandedWasRenamed refuses a change that moves a record which +// was already on the branch the change lands on. +// +// A slug renamed to something better months later is a removal and an addition +// as far as the tree is concerned. Every pointer at the old name stops +// resolving, including a promotion section a reader is following from another +// board, and nothing about the tree afterwards says the old name was ever used. +// +// It is refused rather than repaired, and the refusal names both paths, because +// the repair is a choice this check may not make: keep the slug, or write a new +// experiment with its own question and let the old record say where the work +// went. Renaming in silence is the only option this removes. +const ExperimentAlreadyLandedWasRenamed = "experiment-already-landed-was-renamed" + +// judgeRemovals holds every record that was on the branch at the base of the +// range to still being there at the head. +// +// THE BOUNDARIES, WRITTEN HERE RATHER THAN DISCOVERED. +// +// A record created and removed inside one branch is not covered, and it is not +// covered twice over. Such a record never reached the branch this change lands +// on, so nothing about it was made permanent, and a directory added by mistake +// and taken out again in the same pull request is ordinary work. The range is +// read from base to head, so the file is not in the diff at all, and even where +// something put it there the record would carry no version at the base. +// +// A rename is separated from a removal by what git reported, which is a +// similarity judgement rather than a fact about the change. Where the move is +// not reported as one, and a rename made together with a rewrite of the record +// is the case where it will not be, the refusal is the removal rather than the +// rename. That is red for the right reason and names the wrong repair, which is +// the residual worth knowing about before somebody meets it. +// +// WHAT THIS CANNOT DO is stop history being rewritten on the branch itself. A +// force push that removes the commit the record arrived in is refused by the +// ruleset, which refuses a non-fast-forward push and carries no bypass actors, +// and it is named here so that a green run is not read as covering it. +func judgeRemovals(change Change) Verdict { + if !change.RecordsRead { + return Verdict{Skips: []Skip{ + { + Rule: RecordAlreadyLandedWasRemoved, + Why: "this run was given no records, so nothing was read at the base of the range to be missing at the head", + }, + { + Rule: ExperimentAlreadyLandedWasRenamed, + Why: "this run was given no records, so no record was looked for at the two ends of a move", + }, + }} + } + + var verdict Verdict + if !change.FilesRead { + verdict.Skips = append(verdict.Skips, Skip{ + Rule: ExperimentAlreadyLandedWasRenamed, + Why: "this run was given no changed paths, so a record that moved cannot be told from one that went, and a move is refused as a removal", + }) + } + + moves := renames(change) + for _, record := range change.Records { + if !record.BeforePresent || record.AfterPresent { + continue + } + if to, moved := moves[record.Path]; moved { + verdict.Refusals = append(verdict.Refusals, Refusal{ + Property: ExperimentAlreadyLandedWasRenamed, + Subject: record.Path, + Detail: fmt.Sprintf("it was on the branch this lands on and this change moves it to %s, so every pointer at the old path stops resolving. Keep the slug, or leave the record where it is and let it say where the work went", to), + }) + continue + } + verdict.Refusals = append(verdict.Refusals, Refusal{ + Property: RecordAlreadyLandedWasRemoved, + Subject: record.Path, + Detail: "it was on the branch this lands on and is not at the head of this range, so an experiment that happened stops having happened. Record 0004 removes the code and keeps the record, which gains the line naming the commit that removed it", + }) + } + return verdict +} + +// renames returns where each moved path went, read out of what git reported +// rather than guessed from the content at the two ends. +// +// Guessing was the alternative and it is worse in both directions. Comparing +// the bytes of a record that went with the bytes of one that arrived calls a +// move made together with an edit a removal, and calls two unrelated records +// with the same words a move. +func renames(change Change) map[string]string { + moves := make(map[string]string) + for _, file := range change.Files { + if file.From != "" { + moves[file.From] = file.Path + } + } + return moves +} diff --git a/internal/pullrequest/removal_test.go b/internal/pullrequest/removal_test.go new file mode 100644 index 0000000..15ca671 --- /dev/null +++ b/internal/pullrequest/removal_test.go @@ -0,0 +1,214 @@ +package pullrequest + +import ( + "strings" + "testing" +) + +// The cases over a record that was on the branch a change lands on and is not +// at the head of it. They are returned into the one table in pullrequest_test.go +// rather than run from a second harness, so the proof that every property has a +// case reads them too. +// +// Every case here is one change away from a case that refuses nothing, and the +// one change is always the same field. Whether the record had a version at the +// base is the whole of what separates a removal this rule is about from a +// directory somebody added and took out again while working. +func removalJudgeCases() []judgeCase { + const record = "experiments/one/EXPERIMENT.md" + const moved = "experiments/reading-a-tree-of-records/EXPERIMENT.md" + landed := recordAt("answered", theLandedAnswer) + + return []judgeCase{ + { + name: "a record already on the branch that this change removes", + change: func() Change { + c := clean() + c.Files = []File{ + {Path: record, Gone: true}, + {Path: "experiments/one/measure.go", Gone: true}, + } + c.Records = []RecordChange{{ + Path: record, + Before: landed, + BeforePresent: true, + }} + return c + }(), + // The ordinary shape rather than the malicious one. The prototype + // went under record 0004, what was left looked like an empty + // directory, and tidying it took the record with it. + want: []string{RecordAlreadyLandedWasRemoved}, + }, + { + name: "an experiment directory already on the branch that this change renames", + change: func() Change { + c := clean() + c.Files = []File{ + {Path: record, Gone: true}, + {Path: moved, From: record}, + } + c.Records = []RecordChange{ + {Path: record, Before: landed, BeforePresent: true}, + {Path: moved, After: landed, AfterPresent: true}, + } + return c + }(), + // The record is still in the tree and every pointer at where it + // was has stopped resolving, including a promotion section a + // reader is following from another board. + want: []string{ExperimentAlreadyLandedWasRenamed}, + }, + { + name: "a record this change removes that was never on the branch it lands on", + change: func() Change { + c := clean() + c.Files = []File{{Path: record, Gone: true}} + c.Records = []RecordChange{{Path: record}} + return c + }(), + // The boundary the issue asks for at the check, held in the field + // the rule actually reads. Over a range read from base to head a + // record added and taken out again inside one branch is not in the + // diff at all and never reaches this rule; this case says the rule + // answers the same way where it does reach it, so the boundary does + // not rest on how the range was read. + }, + { + name: "an experiment's code removed with the record kept", + change: func() Change { + c := clean() + c.Files = []File{ + {Path: "experiments/one/measure.go", Gone: true}, + {Path: record}, + } + c.Records = []RecordChange{{ + Path: record, + Before: landed, + BeforePresent: true, + After: recordAt("answered", theLandedAnswer+"\n\n"+ + "The prototype was removed in 1111111111111111111111111111111111111111."), + AfterPresent: true, + }} + return c + }(), + // What record 0004 permits, and the case that says this rule did + // not quietly forbid it. The code goes, the record stays, and it + // gains the line naming the commit that removed it. + }, + } +} + +// TestARemovedRecordNamesWhatMayBeRemovedInstead holds the removal refusal to +// carrying the repair. Somebody meeting this has already decided the directory +// is finished with, and a refusal that only says no sends them looking for +// which of the two rules about records they have hit. +func TestARemovedRecordNamesWhatMayBeRemovedInstead(t *testing.T) { + const record = "experiments/one/EXPERIMENT.md" + + change := clean() + change.Files = []File{{Path: record, Gone: true}} + change.Records = []RecordChange{{ + Path: record, + Before: recordAt("answered", theLandedAnswer), + BeforePresent: true, + }} + + refusals := Judge(change).Refusals + if len(refusals) != 1 { + t.Fatalf("the verdict carries %d refusals, want exactly one", len(refusals)) + } + if refusals[0].Subject != record { + t.Errorf("the refusal names %q, want %q", refusals[0].Subject, record) + } + if !strings.Contains(refusals[0].Detail, "0004") { + t.Errorf("the refusal does not say what may be removed instead: %q", refusals[0].Detail) + } +} + +// TestARenameNamesBothEndsOfTheMove is the half of the issue a property +// identifier cannot carry. The repair is a choice between keeping the slug and +// writing a new experiment, and neither can be made by somebody who has been +// told only where the record used to be. +func TestARenameNamesBothEndsOfTheMove(t *testing.T) { + const from = "experiments/one/EXPERIMENT.md" + const to = "experiments/two/EXPERIMENT.md" + landed := recordAt("answered", theLandedAnswer) + + change := clean() + change.Files = []File{{Path: from, Gone: true}, {Path: to, From: from}} + change.Records = []RecordChange{ + {Path: from, Before: landed, BeforePresent: true}, + {Path: to, After: landed, AfterPresent: true}, + } + + refusals := Judge(change).Refusals + if len(refusals) != 1 { + t.Fatalf("the verdict carries %d refusals, want exactly one", len(refusals)) + } + if refusals[0].Property != ExperimentAlreadyLandedWasRenamed { + t.Fatalf("the verdict refuses %s, want %s", refusals[0].Property, ExperimentAlreadyLandedWasRenamed) + } + if refusals[0].Subject != from { + t.Errorf("the refusal names %q as its subject, want the path the record was at, %q", refusals[0].Subject, from) + } + if !strings.Contains(refusals[0].Detail, to) { + t.Errorf("the refusal never names where the record went: %q", refusals[0].Detail) + } +} + +// TestARenamedRecordSurvivesTheJoinFromTheDiff is the join between the two +// halves of this package, and it was added because deleting the pairing in the +// parser left every case above green. Each half is proved on its own: the cases +// hand the rule a value somebody typed, and the parser is held to what git +// prints. Neither of them notices the pairing being dropped on the way from one +// to the other, which is one field in one branch of ParseFiles and exactly the +// line somebody tidying it would take out. +// +// The diff below is what git prints for a renamed experiment, in the shape +// TestParseFiles already carries. +func TestARenamedRecordSurvivesTheJoinFromTheDiff(t *testing.T) { + const from = "experiments/one/EXPERIMENT.md" + const to = "experiments/two/EXPERIMENT.md" + landed := recordAt("answered", theLandedAnswer) + + files, err := ParseFiles([]byte("R100\x00" + from + "\x00" + to + "\x00")) + if err != nil { + t.Fatalf("cannot read the diff: %v", err) + } + + change := clean() + change.Files = files + change.Records = []RecordChange{ + {Path: from, Before: landed, BeforePresent: true}, + {Path: to, After: landed, AfterPresent: true}, + } + + properties := Judge(change).Properties() + if len(properties) != 1 || properties[0] != ExperimentAlreadyLandedWasRenamed { + t.Fatalf("the verdict is %v, want exactly %s", properties, ExperimentAlreadyLandedWasRenamed) + } +} + +// TestAMoveNothingReportedAsOneIsRefusedAsARemoval holds the residual the rule +// declares. Where git did not call the move a rename, the same change is +// refused for the same reason under the other property, and the repair the +// message names is the wrong one of the two. It is written down here so that a +// reader meeting it knows it was chosen rather than missed. +func TestAMoveNothingReportedAsOneIsRefusedAsARemoval(t *testing.T) { + const from = "experiments/one/EXPERIMENT.md" + const to = "experiments/two/EXPERIMENT.md" + landed := recordAt("answered", theLandedAnswer) + + change := clean() + change.Files = []File{{Path: from, Gone: true}, {Path: to}} + change.Records = []RecordChange{ + {Path: from, Before: landed, BeforePresent: true}, + {Path: to, After: landed, AfterPresent: true}, + } + + properties := Judge(change).Properties() + if len(properties) != 1 || properties[0] != RecordAlreadyLandedWasRemoved { + t.Fatalf("the verdict is %v, want exactly %s", properties, RecordAlreadyLandedWasRemoved) + } +}