Skip to content

Commit

Permalink
rename serie to chain
Browse files Browse the repository at this point in the history
This seems to be a less confusing, as it's already used in other places
using gerrit.
  • Loading branch information
flokli committed Nov 8, 2023
1 parent 150aed4 commit 2d11815
Show file tree
Hide file tree
Showing 10 changed files with 265 additions and 269 deletions.
49 changes: 25 additions & 24 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ effectively makes everybody else rebase again. `gerrit-queue` is meant to
remove these races to master.

Developers can set the `Autosubmit` customized label to `+1` on all changesets
in a series, and if all preconditions on are met ("submittable" in gerrit
in a chain, and if all preconditions on are met ("submittable" in gerrit
speech, this usually means passing CI and passing Code Review),
`gerrit-queue` takes care of rebasing and submitting it to master.

Expand Down Expand Up @@ -44,58 +44,59 @@ but usually developers think in terms of multiple changesets.

### Fetching changesets
`gerrit-queue` fetches all changesets from gerrit, and tries to identify these
chains of changesets. We call them `Series`. All changesets need to have strict
parent/child relationships to be detected (so if only half of the stack gets
rebased by the Gerrit Web interface, these are considered individual series.
chains of changesets.
All changesets need to have strict parent/child relationships to be detected.
(This means, if a user manually rebases half of a chain through the Gerrit Web
Interface, these will be considered as two independent chains!)

Series are sorted by the number of changesets in them. This ensures longer
series are merged faster, and less rebases are triggered. In the future, this
might be extended to other metrics.
Chains are rebased by the number of changesets in them. This ensures longer
chains are merged faster, and less rebases are triggered. In the future, this
might be extended to other strategies.

### Submitting changesets
The submitqueue has a Trigger() function, which gets periodically executed.

It can keep a reference to one single serie across multiple runs. This is
necessary if it previously rebased one serie to current HEAD and needs to wait
It can keep a reference to one single chain across multiple runs. This is
necessary if it previously rebased one chain to current HEAD and needs to wait
some time until CI feedback is there. If it wouldn't keep that state, it would
pick another series (with +1 from CI) and trigger a rebase on that one, so
pick another chain (with +1 from CI) and trigger a rebase on that one, so
depending on CI run times and trigger intervals, if not keepig this information
it'd end up rebasing all unrebased changesets on the same HEAD, and then just
pick one, instead of waiting for the one to finish.

The Trigger() function first instructs the gerrit client to fetch changesets
and assemble series.
If there is a `wipSerie` from a previous run, we check if it can still be found
in the newly assembled list of series (it still needs to contain the same
number of series. Commit IDs may differ, because the code doesn't reassemble a
`wipSerie` after scheduling a rebase.
If the `wipSerie` could be refreshed, we update the pointer with the newly
assembled series. If we couldn't find it, we drop it.
and assemble chains.
If there is a `wipChain` from a previous run, we check if it can still be found
in the newly assembled list of chains (it still needs to contain the same
number of changesets. Commit IDs may differ, because the code doesn't reassemble
a `wipChain` after scheduling a rebase.
If the `wipChain` could be refreshed, we update the pointer with the newly
assembled chain. If we couldn't find it, we drop it.

Now, we enter the main for loop. The first half of the loop checks various
conditions of the current `wipSerie`, and if successful, does the submit
("Submit phase"), the second half will pick a suitable new `wipSerie`, and
conditions of the current `wipChain`, and if successful, does the submit
("Submit phase"), the second half will pick a suitable new `wipChain`, and
potentially do a rebase ("Pick phase").

#### Submit phase
We check if there is an existing `wipSerie`. If there isn't, we immediately go to
We check if there is an existing `wipChain`. If there isn't, we immediately go to
the "pick" phase.

The `wipSerie` still needs to be rebased on `HEAD` (otherwise, the submit queue
The `wipChain` still needs to be rebased on `HEAD` (otherwise, the submit queue
advanced outside of gerrit), and should not fail CI (logical merge conflict) -
otherwise we discard it, and continue with the picking phase.

If the `wipSerie` still contains a changeset awaiting CI feedback, we `return`
If the `wipChain` still contains a changeset awaiting CI feedback, we `return`
from the `Trigger()` function (and go back to sleep).

If the changeset is "submittable" in gerrit speech, and has the necessary
submit queue tag set, we submit it.

#### Pick phase
The pick phase finds a new `wipSerie`. It'll first try to find one that already
The pick phase finds a new `wipChain`. It'll first try to find one that already
is rebased on the current `HEAD` (so the loop can just continue, and the next
submit phase simply submit), and otherwise fall back to a not-yet-rebased
serie. Because the rebase mandates waiting for CI, the code `return`s the
chain. Because the rebase mandates waiting for CI, the code `return`s the
`Trigger()` function, so it'll be called again after waiting some time.

## Compile and Run
Expand Down
12 changes: 6 additions & 6 deletions frontend/frontend.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ import (
//go:embed templates
var templates embed.FS

//loadTemplate loads a list of templates, relative to the templates root, and a
//FuncMap, and returns a template object
// loadTemplate loads a list of templates, relative to the templates root, and a
// FuncMap, and returns a template object
func loadTemplate(templateNames []string, funcMap template.FuncMap) (*template.Template, error) {
if len(templateNames) == 0 {
return nil, fmt.Errorf("templateNames can't be empty")
Expand Down Expand Up @@ -53,13 +53,13 @@ func MakeFrontend(rotatingLogHandler *misc.RotatingLogHandler, gerritClient *ger

mux := http.NewServeMux()
mux.HandleFunc("/", func(w http.ResponseWriter, _ *http.Request) {
var wipSerie *gerrit.Serie = nil
var wipChain *gerrit.Chain = nil
HEAD := ""
currentlyRunning := runner.IsCurrentlyRunning()

// don't trigger operations requiring a lock
if !currentlyRunning {
wipSerie = runner.GetWIPSerie()
wipChain = runner.GetWIPChain()
HEAD = gerritClient.GetHEAD()
}

Expand Down Expand Up @@ -91,7 +91,7 @@ func MakeFrontend(rotatingLogHandler *misc.RotatingLogHandler, gerritClient *ger

tmpl := template.Must(loadTemplate([]string{
"index.tmpl.html",
"serie.tmpl.html",
"chain.tmpl.html",
"changeset.tmpl.html",
}, funcMap))

Expand All @@ -102,7 +102,7 @@ func MakeFrontend(rotatingLogHandler *misc.RotatingLogHandler, gerritClient *ger

// State
"currentlyRunning": currentlyRunning,
"wipSerie": wipSerie,
"wipChain": wipChain,
"HEAD": HEAD,

// History
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{{ define "serie" }}
{{ define "chain" }}
<table class="table table-sm table-hover">
<thead class="thead-light">
<tr>
Expand All @@ -9,7 +9,7 @@
</thead>
<tbody>
<tr>
<td colspan="3" class="table-success">Serie with {{ len .ChangeSets }} changes</td>
<td colspan="3" class="table-success">Chain with {{ len .ChangeSets }} changes</td>
</tr>
{{ range $changeset := .ChangeSets }}
{{ block "changeset" $changeset }}{{ end }}
Expand Down
8 changes: 4 additions & 4 deletions frontend/templates/index.tmpl.html
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
<a class="nav-link" href="#region-info">Info</a>
</li>
<li class="nav-item">
<a class="nav-link" href="#region-wipserie">WIP Serie</a>
<a class="nav-link" href="#region-wipchain">WIP Chain</a>
</li>
<li class="nav-item">
<a class="nav-link" href="#region-log">Log</a>
Expand Down Expand Up @@ -55,9 +55,9 @@ <h2 id="region-info">Info</h2>
</tbody>
</table>

<h2 id="region-wipserie">WIP Serie</h2>
{{ if .wipSerie }}
{{ block "serie" .wipSerie }}{{ end }}
<h2 id="region-wipchain">WIP Chain</h2>
{{ if .wipChain }}
{{ block "chain" .wipChain }}{{ end }}
{{ else }}
-
{{ end }}
Expand Down
42 changes: 19 additions & 23 deletions gerrit/serie.go → gerrit/chain.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,34 +7,34 @@ import (
"github.com/apex/log"
)

// Serie represents a list of successive changesets with an unbroken parent -> child relation,
// Chain represents a list of successive changesets with an unbroken parent -> child relation,
// starting from the parent.
type Serie struct {
type Chain struct {
ChangeSets []*Changeset
}

// GetParentCommitIDs returns the parent commit IDs
func (s *Serie) GetParentCommitIDs() ([]string, error) {
func (s *Chain) GetParentCommitIDs() ([]string, error) {
if len(s.ChangeSets) == 0 {
return nil, fmt.Errorf("Can't return parent on a serie with zero ChangeSets")
return nil, fmt.Errorf("can't return parent on a chain with zero ChangeSets")
}
return s.ChangeSets[0].ParentCommitIDs, nil
}

// GetLeafCommitID returns the commit id of the last commit in ChangeSets
func (s *Serie) GetLeafCommitID() (string, error) {
func (s *Chain) GetLeafCommitID() (string, error) {
if len(s.ChangeSets) == 0 {
return "", fmt.Errorf("Can't return leaf on a serie with zero ChangeSets")
return "", fmt.Errorf("can't return leaf on a chain with zero ChangeSets")
}
return s.ChangeSets[len(s.ChangeSets)-1].CommitID, nil
}

// CheckIntegrity checks that the series contains a properly ordered and connected chain of commits
func (s *Serie) CheckIntegrity() error {
logger := log.WithField("serie", s)
// an empty serie is invalid
// Validate checks that the chain contains a properly ordered and connected chain of commits
func (s *Chain) Validate() error {
logger := log.WithField("chain", s)
// an empty chain is invalid
if len(s.ChangeSets) == 0 {
return fmt.Errorf("An empty serie is invalid")
return fmt.Errorf("an empty chain is invalid")
}

previousCommitID := ""
Expand All @@ -48,12 +48,12 @@ func (s *Serie) CheckIntegrity() error {

parentCommitIDs := changeset.ParentCommitIDs
if len(parentCommitIDs) == 0 {
return fmt.Errorf("Changesets without any parent are not supported")
return fmt.Errorf("changesets without any parent are not supported")
}
// we don't check parents of the first changeset in a series
// we don't check parents of the first changeset in a chain
if i != 0 {
if len(parentCommitIDs) != 1 {
return fmt.Errorf("Merge commits in the middle of a series are not supported (only at the beginning)")
return fmt.Errorf("merge commits in the middle of a chain are not supported (only at the beginning)")
}
if parentCommitIDs[0] != previousCommitID {
return fmt.Errorf("changesets parent commit id doesn't match previous commit id")
Expand All @@ -65,20 +65,20 @@ func (s *Serie) CheckIntegrity() error {
return nil
}

// FilterAllChangesets applies a filter function on all of the changesets in the series.
// AllChangesets applies a filter function on all of the changesets in the chain.
// returns true if it returns true for all changesets, false otherwise
func (s *Serie) FilterAllChangesets(f func(c *Changeset) bool) bool {
func (s *Chain) AllChangesets(f func(c *Changeset) bool) bool {
for _, changeset := range s.ChangeSets {
if f(changeset) == false {
if !f(changeset) {
return false
}
}
return true
}

func (s *Serie) String() string {
func (s *Chain) String() string {
var sb strings.Builder
sb.WriteString(fmt.Sprintf("Serie[%d]", len(s.ChangeSets)))
sb.WriteString(fmt.Sprintf("Chain[%d]", len(s.ChangeSets)))
if len(s.ChangeSets) == 0 {
sb.WriteString("()\n")
return sb.String()
Expand Down Expand Up @@ -106,7 +106,3 @@ func (s *Serie) String() string {
s.ChangeSets[len(s.ChangeSets)-1].CommitID))
return sb.String()
}

func shortCommitID(commitID string) string {
return commitID[:6]
}
125 changes: 125 additions & 0 deletions gerrit/chains.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
package gerrit

import (
"sort"

"github.com/apex/log"
)

// AssembleChain consumes a list of changesets, and groups them together to chains.
//
// Initially, every changeset is put in its own individual chain.
//
// we maintain a lookup table, mapLeafToChain,
// which allows to lookup a chain by its leaf commit id
// We concat chains in a fixpoint approach
// because both appending and prepending is much more complex.
// Concatenation moves changesets of the later changeset in the previous one
// in a cleanup phase, we remove orphaned chains (those without any changesets inside)
// afterwards, we do an integrity check, just to be on the safe side.
func AssembleChain(changesets []*Changeset, logger *log.Logger) ([]*Chain, error) {
chains := make([]*Chain, 0)
mapLeafToChain := make(map[string]*Chain, 0)

for _, changeset := range changesets {
l := logger.WithField("changeset", changeset.String())

l.Debug("creating initial chain")
chain := &Chain{
ChangeSets: []*Changeset{changeset},
}
chains = append(chains, chain)
mapLeafToChain[changeset.CommitID] = chain
}

// Combine chain using a fixpoint approach, with a max iteration count.
logger.Debug("glueing together phase")
for i := 1; i < 100; i++ {
didUpdate := false
logger.Debugf("at iteration %d", i)
for j, chain := range chains {
l := logger.WithFields(log.Fields{
"i": i,
"j": j,
"chain": chain.String(),
})
parentCommitIDs, err := chain.GetParentCommitIDs()
if err != nil {
return chains, err
}
if len(parentCommitIDs) != 1 {
// We can't append merge commits to other chains
l.Infof("No single parent, skipping.")
continue
}
parentCommitID := parentCommitIDs[0]
l.Debug("Looking for a predecessor.")
// if there's another chain that has this parent as a leaf, glue together
if otherChain, ok := mapLeafToChain[parentCommitID]; ok {
if otherChain == chain {
continue
}
l = l.WithField("otherChain", otherChain)

myLeafCommitID, err := chain.GetLeafCommitID()
if err != nil {
return chains, err
}

// append our changesets to the other chain
l.Debug("Splicing together.")
otherChain.ChangeSets = append(otherChain.ChangeSets, chain.ChangeSets...)

delete(mapLeafToChain, parentCommitID)
mapLeafToChain[myLeafCommitID] = otherChain

// orphan our chain
chain.ChangeSets = []*Changeset{}
// remove the orphaned chain from the lookup table
delete(mapLeafToChain, myLeafCommitID)

didUpdate = true
} else {
l.Debug("Not found.")
}
}
chains = removeOrphanedChains(chains)
if !didUpdate {
logger.Infof("converged after %d iterations", i)
break
}
}

// Check integrity, just to be on the safe side.
for _, chain := range chains {
l := logger.WithField("chain", chain.String())
l.Debugf("checking integrity")
err := chain.Validate()
if err != nil {
l.Errorf("checking integrity failed: %s", err)
}
}
return chains, nil
}

// removeOrphanedChains removes all empty chains (that contain zero changesets)
func removeOrphanedChains(chains []*Chain) []*Chain {
newChains := []*Chain{}
for _, chain := range chains {
if len(chain.ChangeSets) != 0 {
newChains = append(newChains, chain)
}
}
return newChains
}

// SortChains sorts a list of chains by the number of changesets in each chain, descending
func SortChains(chains []*Chain) []*Chain {
newChains := make([]*Chain, len(chains))
copy(newChains, chains)
sort.Slice(newChains, func(i, j int) bool {
// the weight depends on the amount of changesets in the chain
return len(chains[i].ChangeSets) > len(chains[j].ChangeSets)
})
return newChains
}
Loading

0 comments on commit 2d11815

Please sign in to comment.