Skip to content

Commit 06211d8

Browse files
committed
docs: define the URI as the unit of change
## Summary ### Why? The contract never said what a URI is relative to a change, and the two readings lead to different behavior. Under one, a change is the unit and its URIs are pieces of it; under the other, each URI is a change and a list is a stack of them. Nothing wrote the second one down, so the first kept getting assumed — most recently in SQUASH_REBASE, which collapsed every URI of a step into one commit and erased the boundary between stacked pull requests. The strategy fields had the same hole. Nothing said whether a strategy is picked once and repeated per URI, or picked once for the list as a whole. Both `LandRequest.strategy` and `MergeStep.strategy` are singular, which reads either way. ### What? States the rule once where a change is defined, in `uber.base.change.Change` and its `platform/base/change` entity: one URI is one unit of change, and a list is an ordered set of distinct changes applied each on top of the last, not one change described several ways. Carries it to the places a strategy is chosen. The `Strategy` enum now says a strategy applies to every URI the change carries, the same way to each, and its values are defined per URI — so `SQUASH_REBASE` says the squash unit is the individual change, and a stack of three URIs becomes three commits rather than one. `PROMOTE` notes that it is the one value constraining the list rather than repeating over it: advancing a ref to an exact revision admits a single URI, because a second could not also be the revision the target ends at. The two call sites say the same in their own terms — `MergeStep.strategy` that a step is never a mix of strategies, `LandRequest.strategy` that a land request cannot pick a different strategy per URI — as do the `LandStrategy` entity fields. The git merger's README gains the corresponding implementation statement: the URI is the unit of application, and a step's outputs are the concatenation of what each URI produced. Documentation only. The behavior described is what the code already does; this is the contract catching up with it, so that the next reader does not have to infer the rule from an implementation. ## Test Plan ✅ `make proto` — regenerated stubs carry the new comments ✅ `bazel test //runway/... //service/runway/... //submitqueue/...` — 47/47 ✅ `make lint`, `make check-tidy`, `make check-gazelle`, `make test` No behavior change, so no new tests. The rule stated here is already pinned by existing cases — `TestMerge_SquashRebase_OneCommitPerChange` for the per-URI squash unit, `TestMerge_RejectsInconsistentProvider` for one provider per request, and the PROMOTE composition cases for its single-URI constraint.
1 parent 3f6b3bc commit 06211d8

12 files changed

Lines changed: 76 additions & 12 deletions

File tree

api/base/change/proto/change.proto

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,5 +41,13 @@ message Change {
4141
//
4242
// The commit SHA must be the full 40-character lowercase hex SHA; abbreviated
4343
// SHAs are rejected because downstream staleness checks compare by strict equality.
44+
//
45+
// One URI is one unit of change -- a single pull request, revision, or
46+
// commit -- and it is the granularity everything downstream operates at. A
47+
// list is therefore an ordered set of distinct changes (a stack), not one
48+
// change described several ways: they are applied in the order given, each
49+
// on top of the last, and each yields its own result. An integration
50+
// strategy chosen for a change applies to every URI in the list, the same
51+
// way to each.
4452
repeated string uris = 1;
4553
}

api/base/change/protopb/change.pb.go

Lines changed: 8 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

api/base/mergestrategy/proto/mergestrategy.proto

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,17 +27,30 @@ option java_package = "com.uber.submitqueue.base.mergestrategy";
2727
// This is the shared wire contract for merge strategy, reused across SubmitQueue
2828
// and other repo-local domains — the proto-level analog of the
2929
// platform/base/mergestrategy Go entity. Domains import it rather than redefining their own.
30+
//
31+
// A strategy is chosen once for a change and applies to every URI that change
32+
// carries, the same way to each. Since one URI is one unit of change (see
33+
// uber.base.change.Change), the values below are defined per URI: a change
34+
// carrying three URIs is integrated as three separate applications of the same
35+
// strategy, in order, not as one combined application.
3036
enum Strategy {
3137
// Default strategy (let the server decide based on configuration).
3238
DEFAULT = 0;
33-
// Rebase commits onto the target branch before landing.
39+
// Rebase the commits the change introduces onto the target branch before landing.
3440
REBASE = 1;
35-
// Same as REBASE but squash commits into a single commit before rebase.
41+
// Same as REBASE, then squash into a single commit. The squash unit is the
42+
// individual change: a change of ten commits becomes one commit, and a
43+
// stack of three URIs becomes three commits rather than one, so the
44+
// boundary between the stacked changes survives.
3645
SQUASH_REBASE = 2;
37-
// Merge commits into the target branch by creating a separate merge commit, preserving commit history along with hashes.
46+
// Merge into the target branch by creating a separate merge commit, preserving commit history along with hashes.
3847
MERGE = 3;
3948
// Integrate the exact revision as-is, with no content transform — advance the target branch to an already-existing
4049
// commit rather than producing new revisions. The implementer maps it to its backend: git fast-forward, Mercurial
4150
// bookmark advance, Subversion/Perforce copy. Used to promote an already-landed/verified commit onto another branch.
51+
//
52+
// The one strategy that constrains the list rather than repeating over it:
53+
// advancing a ref to an exact revision admits a single URI, because a
54+
// second one could not also be the revision the target ends at.
4255
PROMOTE = 4;
4356
}

api/base/mergestrategy/protopb/mergestrategy.pb.go

Lines changed: 16 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

api/runway/messagequeue/proto/merge.proto

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,14 @@ message MergeStep {
3636
// StepResult so a multi-step result is attributable -- and never interprets
3737
// its contents.
3838
string step_id = 1;
39-
// change is the code change to apply for this step. A change may carry
40-
// multiple URIs when the step represents stacked or grouped changes.
39+
// change is the code change to apply for this step. It may carry multiple
40+
// URIs, which are a stack: each URI is its own unit of change, applied in
41+
// the order given, each on top of the last.
4142
uber.base.change.Change change = 2;
4243
// strategy is how this step's change is integrated into the merge target.
44+
// It applies to every URI the change carries, the same way to each, so a
45+
// step is never a mix of strategies. PROMOTE is the exception that admits
46+
// only a single URI.
4347
uber.base.mergestrategy.Strategy strategy = 3;
4448
}
4549

api/runway/messagequeue/protopb/merge.pb.go

Lines changed: 6 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

api/submitqueue/gateway/proto/gateway.proto

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,8 @@ message LandRequest {
5757
// Change (such as a pull request) to land into the target branch. Target branch is defined by the queue configuration.
5858
uber.base.change.Change change = 2;
5959
// Source control integration strategy to use for this land operation. If not specified, the default queue strategy is used.
60+
// It applies to every URI the change carries, the same way to each — a land
61+
// request cannot pick a different strategy per URI.
6062
uber.base.mergestrategy.Strategy strategy = 4;
6163
}
6264

api/submitqueue/gateway/protopb/gateway.pb.go

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

platform/base/change/change.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,5 +32,11 @@ type Change struct {
3232
//
3333
// Head/commit SHAs must be the full 40-char lowercase hex form.
3434
//
35+
// One URI is one unit of change — a single pull request, revision, or
36+
// commit. A list is an ordered set of distinct changes (a stack), not one
37+
// change described several ways: order is significant, each entry applies
38+
// on top of the last, and each yields its own result. An integration
39+
// strategy chosen for a change applies to every URI in the list, the same
40+
// way to each.
3541
URIs []string `json:"uris"`
3642
}

runway/extension/merger/git/README.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ A `merger.Merger` backed by the `git` CLI operating on a local checkout. It appl
66

77
A request is an ordered list of steps; each step names a change (a set of provider URIs, each ending in a full head commit SHA) and a strategy. Steps are applied in order on top of the target tip — earlier steps are the in-flight base, the last step is the candidate. Each step yields one `StepResult`; the revisions a step produces on the target are its outputs, in application order.
88

9+
The URI is the unit of application. A step's change may carry several URIs — a stack — and the step's strategy applies to each of them, the same way to each, in the order given. So a step is never a mix of strategies, and its outputs are the concatenation of what each URI produced: one revision per created commit under `REBASE`, one per URI under `SQUASH_REBASE` and `MERGE`. `PROMOTE` is the exception that admits only one URI, since advancing a ref to an exact revision cannot repeat.
10+
911
A URI pins a change to one head commit, but a change is routinely several commits. The full set is recovered locally rather than from the wire: the commits to replay are the range from the change's merge base with the target up to its head. Applying the head commit alone would apply only that commit's diff against its own parent — conflicting against context its predecessors would have established, or silently dropping them when they touch different files.
1012

1113
## Change providers

0 commit comments

Comments
 (0)