Skip to content

Commit

Permalink
Fix rebase when changeIDs are missing
Browse files Browse the repository at this point in the history
Context
---

When git-review is run a first time when the git hook is not installed,
maiao wil do 2 things:

1. suggest to install the git hook
2. perform a rebase so all commits are updated with a changeID

When establishing the list of commits, maiao walks through the log,
from the most recent to the oldest, starting from the repo HEAD
and ending with the remote HEAD.

Those commits are then added to the changes in the same order so they
can be used in the rebase TODO list in the relevant order.

Problem
---

The technique used to inject the changes differs whether there is a
changeID or not in the commit message.

If a changeID is present, the commit is added at the beginning of the
change list, ensuring an order from oldest to newest.
If now, the change ID is absent, the commit is added at the end of the
list, making an order from newest to oldest.

Concretely, if the local changes are:

* $sha1 A commit without changeID 1
* $sha2 A commit with a changeID 1
* $sha3 A commit without changeID 2
* $sha4 A commit with a changeID 2

They will bet rebased as following, which may cause rebase conflicts on
the way.

* $sha1 A commit without changeID 1
* $sha3 A commit without changeID 2
* $sha2 A commit with a changeID 1
* $sha4 A commit with a changeID 2

Goal
---

Fix this situation

Change-Id: Id8ee8c9a86ba7081bddc123a31adbca382445582
  • Loading branch information
Thibault Jamet committed Mar 12, 2024
1 parent ae70280 commit e4a3d6e
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 2 deletions.
4 changes: 2 additions & 2 deletions pkg/maiao/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -395,11 +395,11 @@ func extractChanges(ctx context.Context, repo lgit.Repository, base, head plumbi
}
changeID, ok := message.GetChangeID()
if !ok {
changes = append(changes, &change{
changes = append([]*change{{
commits: changeCommits,
head: changeCommits[len(changeCommits)-1],
message: message,
})
}}, changes...)

} else {
changes = append([]*change{{
Expand Down
74 changes: 74 additions & 0 deletions pkg/maiao/review_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,18 @@ import (
"bytes"
"context"
"errors"
"fmt"
"io"
"io/ioutil"
"os"
"os/exec"
"strings"
"testing"

"github.com/adevinta/maiao/pkg/api"
"github.com/adevinta/maiao/pkg/log"
"github.com/adevinta/maiao/pkg/system"
"github.com/spf13/afero"

"github.com/go-git/go-git/v5"
"github.com/go-git/go-git/v5/config"
Expand Down Expand Up @@ -323,6 +328,75 @@ func TestRemoveMergedChangeIDs(t *testing.T) {
)
}

func gitCommand(t *testing.T, path string, args ...string) string {
t.Helper()
cmd := exec.Command("git", args...)
cmd.Dir = path
out, err := cmd.CombinedOutput()
require.NoError(t, err)
return strings.TrimSpace(string(out))
}

func TestExtractChanges(t *testing.T) {
d, err := os.MkdirTemp("", t.Name())
require.NoError(t, err)
t.Cleanup(func() {
os.RemoveAll(d)
})
fs := afero.NewBasePathFs(afero.NewOsFs(), d)

gitCommand(t, d, "init")
gitCommand(t, d, "config", "user.email", "john.doe@example.com")
gitCommand(t, d, "config", "user.name", "John Doe")
gitCommand(t, d, "commit", "--allow-empty", "-m", "Initial commit")
initialCommit := gitCommand(t, d, "rev-parse", "HEAD")
system.EnsureTestFileContent(t, fs, "code.txt", `
func removeMergedChangeIDs(changes []*change, knownChangeIDs map[string]struct{}) []*change {
filtered := []*change{}
for _, change := range changes {
if _, ok := knownChangeIDs[change.changeID]; !ok {
filtered = append(filtered, change)
}
}
return filtered
}`)
gitCommand(t, d, "add", "code.txt")
gitCommand(t, d, "commit", "-m", "Code number one")
system.EnsureTestFileContent(t, fs, "code.txt", `
func removeMergedChangeIDs(changes []*change, knownChangeIDs map[string]struct{}) []*change {
filtered := []*change{}
for _, change := range changes {
}
return filtered
}`)
gitCommand(t, d, "add", "code.txt")
gitCommand(t, d, "commit", "-m", "Code number two")
system.EnsureTestFileContent(t, fs, "code.txt", `
func removeMergedChangeIDs(changes []*change, knownChangeIDs map[string]struct{}) []*change {
filtered := []*change{}
for _, change := range changes {
fmt.Println(change)
}
return filtered
}`)
gitCommand(t, d, "add", "code.txt")
gitCommand(t, d, "commit", "-m", "Code number three", "-m", "ChangeId: I1234")
fmt.Println(d)

repo, err := git.PlainOpen(d)
require.NoError(t, err)
head, err := repo.Head()
require.NoError(t, err)
b, err := repo.ResolveRevision(plumbing.Revision(initialCommit))
require.NoError(t, err)
changes, err := extractChanges(context.Background(), repo, *b, head.Hash())
require.NoError(t, err)
require.Len(t, changes, 3)
assert.Equal(t, "Code number one", strings.TrimSpace(changes[0].commits[0].Message))
assert.Equal(t, "Code number two", strings.TrimSpace(changes[1].commits[0].Message))
assert.Equal(t, "Code number three\n\nChangeId: I1234", strings.TrimSpace(changes[2].commits[0].Message))
}

func TestNeedReview(t *testing.T) {
storage := memory.NewStorage()
rootParent := "bdc945b1bc57b3938f7223c7adb8bc2db58b838f"
Expand Down

0 comments on commit e4a3d6e

Please sign in to comment.