From a4fbfd9eea8f690e89e145f251ec06e3bb51bbd2 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Thu, 30 Jul 2026 12:56:32 -0700 Subject: [PATCH] docs: define the URI as the unit of change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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. --- api/base/change/proto/change.proto | 8 ++++++++ api/base/change/protopb/change.pb.go | 8 ++++++++ .../mergestrategy/proto/mergestrategy.proto | 19 ++++++++++++++++--- .../mergestrategy/protopb/mergestrategy.pb.go | 19 ++++++++++++++++--- api/runway/messagequeue/proto/merge.proto | 8 ++++++-- api/runway/messagequeue/protopb/merge.pb.go | 8 ++++++-- api/submitqueue/gateway/proto/gateway.proto | 2 ++ api/submitqueue/gateway/protopb/gateway.pb.go | 2 ++ platform/base/change/change.go | 6 ++++++ runway/extension/merger/git/README.md | 2 ++ submitqueue/entity/land.go | 3 ++- submitqueue/entity/request.go | 3 ++- 12 files changed, 76 insertions(+), 12 deletions(-) diff --git a/api/base/change/proto/change.proto b/api/base/change/proto/change.proto index 784eaa1b..a2d7aa9e 100644 --- a/api/base/change/proto/change.proto +++ b/api/base/change/proto/change.proto @@ -41,5 +41,13 @@ message Change { // // The commit SHA must be the full 40-character lowercase hex SHA; abbreviated // SHAs are rejected because downstream staleness checks compare by strict equality. + // + // One URI is one unit of change -- a single pull request, revision, or + // commit -- and it is the granularity everything downstream operates at. A + // list is therefore an ordered set of distinct changes (a stack), not one + // change described several ways: they are applied in the order given, each + // on top of the last, and each yields its own result. An integration + // strategy chosen for a change applies to every URI in the list, the same + // way to each. repeated string uris = 1; } diff --git a/api/base/change/protopb/change.pb.go b/api/base/change/protopb/change.pb.go index 37c95cad..50b2fdcf 100644 --- a/api/base/change/protopb/change.pb.go +++ b/api/base/change/protopb/change.pb.go @@ -58,6 +58,14 @@ type Change struct { // // The commit SHA must be the full 40-character lowercase hex SHA; abbreviated // SHAs are rejected because downstream staleness checks compare by strict equality. + // + // One URI is one unit of change -- a single pull request, revision, or + // commit -- and it is the granularity everything downstream operates at. A + // list is therefore an ordered set of distinct changes (a stack), not one + // change described several ways: they are applied in the order given, each + // on top of the last, and each yields its own result. An integration + // strategy chosen for a change applies to every URI in the list, the same + // way to each. Uris []string `protobuf:"bytes,1,rep,name=uris,proto3" json:"uris,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache diff --git a/api/base/mergestrategy/proto/mergestrategy.proto b/api/base/mergestrategy/proto/mergestrategy.proto index 4a17e6a4..0037ffaa 100644 --- a/api/base/mergestrategy/proto/mergestrategy.proto +++ b/api/base/mergestrategy/proto/mergestrategy.proto @@ -27,17 +27,30 @@ option java_package = "com.uber.submitqueue.base.mergestrategy"; // This is the shared wire contract for merge strategy, reused across SubmitQueue // and other repo-local domains — the proto-level analog of the // platform/base/mergestrategy Go entity. Domains import it rather than redefining their own. +// +// A strategy is chosen once for a change and applies to every URI that change +// carries, the same way to each. Since one URI is one unit of change (see +// uber.base.change.Change), the values below are defined per URI: a change +// carrying three URIs is integrated as three separate applications of the same +// strategy, in order, not as one combined application. enum Strategy { // Default strategy (let the server decide based on configuration). DEFAULT = 0; - // Rebase commits onto the target branch before landing. + // Rebase the commits the change introduces onto the target branch before landing. REBASE = 1; - // Same as REBASE but squash commits into a single commit before rebase. + // Same as REBASE, then squash into a single commit. The squash unit is the + // individual change: a change of ten commits becomes one commit, and a + // stack of three URIs becomes three commits rather than one, so the + // boundary between the stacked changes survives. SQUASH_REBASE = 2; - // Merge commits into the target branch by creating a separate merge commit, preserving commit history along with hashes. + // Merge into the target branch by creating a separate merge commit, preserving commit history along with hashes. MERGE = 3; // Integrate the exact revision as-is, with no content transform — advance the target branch to an already-existing // commit rather than producing new revisions. The implementer maps it to its backend: git fast-forward, Mercurial // bookmark advance, Subversion/Perforce copy. Used to promote an already-landed/verified commit onto another branch. + // + // The one strategy that constrains the list rather than repeating over it: + // advancing a ref to an exact revision admits a single URI, because a + // second one could not also be the revision the target ends at. PROMOTE = 4; } diff --git a/api/base/mergestrategy/protopb/mergestrategy.pb.go b/api/base/mergestrategy/protopb/mergestrategy.pb.go index 022f8d0f..02606748 100644 --- a/api/base/mergestrategy/protopb/mergestrategy.pb.go +++ b/api/base/mergestrategy/protopb/mergestrategy.pb.go @@ -42,20 +42,33 @@ const ( // This is the shared wire contract for merge strategy, reused across SubmitQueue // and other repo-local domains — the proto-level analog of the // platform/base/mergestrategy Go entity. Domains import it rather than redefining their own. +// +// A strategy is chosen once for a change and applies to every URI that change +// carries, the same way to each. Since one URI is one unit of change (see +// uber.base.change.Change), the values below are defined per URI: a change +// carrying three URIs is integrated as three separate applications of the same +// strategy, in order, not as one combined application. type Strategy int32 const ( // Default strategy (let the server decide based on configuration). Strategy_DEFAULT Strategy = 0 - // Rebase commits onto the target branch before landing. + // Rebase the commits the change introduces onto the target branch before landing. Strategy_REBASE Strategy = 1 - // Same as REBASE but squash commits into a single commit before rebase. + // Same as REBASE, then squash into a single commit. The squash unit is the + // individual change: a change of ten commits becomes one commit, and a + // stack of three URIs becomes three commits rather than one, so the + // boundary between the stacked changes survives. Strategy_SQUASH_REBASE Strategy = 2 - // Merge commits into the target branch by creating a separate merge commit, preserving commit history along with hashes. + // Merge into the target branch by creating a separate merge commit, preserving commit history along with hashes. Strategy_MERGE Strategy = 3 // Integrate the exact revision as-is, with no content transform — advance the target branch to an already-existing // commit rather than producing new revisions. The implementer maps it to its backend: git fast-forward, Mercurial // bookmark advance, Subversion/Perforce copy. Used to promote an already-landed/verified commit onto another branch. + // + // The one strategy that constrains the list rather than repeating over it: + // advancing a ref to an exact revision admits a single URI, because a + // second one could not also be the revision the target ends at. Strategy_PROMOTE Strategy = 4 ) diff --git a/api/runway/messagequeue/proto/merge.proto b/api/runway/messagequeue/proto/merge.proto index acbd800b..babd70ab 100644 --- a/api/runway/messagequeue/proto/merge.proto +++ b/api/runway/messagequeue/proto/merge.proto @@ -36,10 +36,14 @@ message MergeStep { // StepResult so a multi-step result is attributable -- and never interprets // its contents. string step_id = 1; - // change is the code change to apply for this step. A change may carry - // multiple URIs when the step represents stacked or grouped changes. + // change is the code change to apply for this step. It may carry multiple + // URIs, which are a stack: each URI is its own unit of change, applied in + // the order given, each on top of the last. uber.base.change.Change change = 2; // strategy is how this step's change is integrated into the merge target. + // It applies to every URI the change carries, the same way to each, so a + // step is never a mix of strategies. PROMOTE is the exception that admits + // only a single URI. uber.base.mergestrategy.Strategy strategy = 3; } diff --git a/api/runway/messagequeue/protopb/merge.pb.go b/api/runway/messagequeue/protopb/merge.pb.go index 07608874..668e26e3 100644 --- a/api/runway/messagequeue/protopb/merge.pb.go +++ b/api/runway/messagequeue/protopb/merge.pb.go @@ -107,10 +107,14 @@ type MergeStep struct { // StepResult so a multi-step result is attributable -- and never interprets // its contents. StepId string `protobuf:"bytes,1,opt,name=step_id,json=stepId,proto3" json:"step_id,omitempty"` - // change is the code change to apply for this step. A change may carry - // multiple URIs when the step represents stacked or grouped changes. + // change is the code change to apply for this step. It may carry multiple + // URIs, which are a stack: each URI is its own unit of change, applied in + // the order given, each on top of the last. Change *protopb.Change `protobuf:"bytes,2,opt,name=change,proto3" json:"change,omitempty"` // strategy is how this step's change is integrated into the merge target. + // It applies to every URI the change carries, the same way to each, so a + // step is never a mix of strategies. PROMOTE is the exception that admits + // only a single URI. Strategy protopb1.Strategy `protobuf:"varint,3,opt,name=strategy,proto3,enum=uber.base.mergestrategy.Strategy" json:"strategy,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache diff --git a/api/submitqueue/gateway/proto/gateway.proto b/api/submitqueue/gateway/proto/gateway.proto index ad751602..3c571e80 100644 --- a/api/submitqueue/gateway/proto/gateway.proto +++ b/api/submitqueue/gateway/proto/gateway.proto @@ -57,6 +57,8 @@ message LandRequest { // Change (such as a pull request) to land into the target branch. Target branch is defined by the queue configuration. uber.base.change.Change change = 2; // Source control integration strategy to use for this land operation. If not specified, the default queue strategy is used. + // It applies to every URI the change carries, the same way to each — a land + // request cannot pick a different strategy per URI. uber.base.mergestrategy.Strategy strategy = 4; } diff --git a/api/submitqueue/gateway/protopb/gateway.pb.go b/api/submitqueue/gateway/protopb/gateway.pb.go index 9988e24b..61c7328d 100644 --- a/api/submitqueue/gateway/protopb/gateway.pb.go +++ b/api/submitqueue/gateway/protopb/gateway.pb.go @@ -169,6 +169,8 @@ type LandRequest struct { // Change (such as a pull request) to land into the target branch. Target branch is defined by the queue configuration. Change *protopb.Change `protobuf:"bytes,2,opt,name=change,proto3" json:"change,omitempty"` // Source control integration strategy to use for this land operation. If not specified, the default queue strategy is used. + // It applies to every URI the change carries, the same way to each — a land + // request cannot pick a different strategy per URI. Strategy protopb1.Strategy `protobuf:"varint,4,opt,name=strategy,proto3,enum=uber.base.mergestrategy.Strategy" json:"strategy,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache diff --git a/platform/base/change/change.go b/platform/base/change/change.go index 5848948d..1d430d9a 100644 --- a/platform/base/change/change.go +++ b/platform/base/change/change.go @@ -32,5 +32,11 @@ type Change struct { // // Head/commit SHAs must be the full 40-char lowercase hex form. // + // One URI is one unit of change — a single pull request, revision, or + // commit. A list is an ordered set of distinct changes (a stack), not one + // change described several ways: order is significant, each entry applies + // on top of the last, and each yields its own result. An integration + // strategy chosen for a change applies to every URI in the list, the same + // way to each. URIs []string `json:"uris"` } diff --git a/runway/extension/merger/git/README.md b/runway/extension/merger/git/README.md index 4911ca98..d381567d 100644 --- a/runway/extension/merger/git/README.md +++ b/runway/extension/merger/git/README.md @@ -6,6 +6,8 @@ A `merger.Merger` backed by the `git` CLI operating on a local checkout. It appl 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. +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. + 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. ## Change providers diff --git a/submitqueue/entity/land.go b/submitqueue/entity/land.go index e0c96e17..edeaa60a 100644 --- a/submitqueue/entity/land.go +++ b/submitqueue/entity/land.go @@ -31,7 +31,8 @@ type LandRequest struct { Queue string `json:"queue"` // Change is the set of code changes to land. Change change.Change `json:"change"` - // LandStrategy is the source control integration strategy to use for this land operation. + // LandStrategy is the source control integration strategy to use for this + // land operation. It applies to every URI of Change, the same way to each. LandStrategy mergestrategy.MergeStrategy `json:"land_strategy"` } diff --git a/submitqueue/entity/request.go b/submitqueue/entity/request.go index 0b88a25b..d741d1bc 100644 --- a/submitqueue/entity/request.go +++ b/submitqueue/entity/request.go @@ -82,7 +82,8 @@ type Request struct { Queue string `json:"queue"` // Change is a number of code changes (such as pull requests) to land into the target branch. Target branch is defined by the queue configuration. Change change.Change `json:"change"` - // LandStrategy is the source control integration strategy to use for this land operation. + // LandStrategy is the source control integration strategy to use for this + // land operation. It applies to every URI of Change, the same way to each. LandStrategy mergestrategy.MergeStrategy `json:"land_strategy"` // ****************