Message abstraction level

For https://gitlab.com/gitlab-org/gitaly/issues/127 I'm developing a new FindLocalBranches rpc call, that for efficiency has to return a good amount of data about branches. In Rugged, this data is abstracted under several layers. I was wondering if for this type of cases, we should replicate these abstractions in our messages, with something like:

message FindLocalBranchesResponse {
  repeated Branch branches = 1;
}

message Branch {
  bytes name = 1;
  Commit head = 2;
}

message Commit {
  string id = 1
  bytes message = 2
  CommitAuthor author = 3
}

message CommitAuthor {
  string date = 1
  // ...
}

Or be more plain with something like:

message FindLocalBranchesResponse {
  repeated Branch branches = 1;
}

message Branch {
  bytes name = 1;
  string commitId = 2;
  bytes commitMessage = 3;
  string commitAuthorDate = 4;
  // ...
}

Also, since our guideline is to have granular messages, should the name of intermediary message be specific to each RPC call (i.e. FindLocalBranchesBranch instead of Branch)?

/cc @gl-gitaly

Well, we have message Repository already, so message Branch makes sense I guess. As for plain/verbose, unsure. Technically a (generic) Commit also includes a diff and parents and stuff that might pop-up in the future. OTOH a Branch is generic enough with just a name and a commit.

As for this specific usecase, what data do you need now for FindLocalBranches? So you need message and author-date? could we have a separate call that takes a repeated bytes commit_id that returns bytes message & string authorDate to get this when needed? (Since I don't know which Controller this is for I don't know what data you need )

Edit: With that being said, CommitMsg and CommitAuthorDate does not belong in a generic message Branch

Repository is inbound, Branch is outbound. I wouldn't use 'we also have Repository' as an argument here.

@eReGeBe I think we shouldn't make our life complicated by inventing a lot of types just to respond to one RPC.

Would this make sense?

message FindLocalBranchesResponse {
  repeated FindLocalBranchResponse branches = 1;
}

message FindLocalBranchResponse {
  // flat struct with the fields we need
}

@jacobvosmaer-gitlab this is what the flat struct would look like with all the data needed:

message FindLocalBranchResponse {
  bytes name = 1;
  string commitId = 2;
  bytes commitMessage = 3;
  string commitAuthorName = 4;
  string commitAuthorEmail = 5;
  Timestamp commitAuthorDate = 6;
  string commitCommitterName = 7;
  string commitCommitterEmail = 8;
  Timestamp commitCommitterDate = 9;
}

This is probably ok, but it could be nicer if there was a message FindLocalBranchCommitAuthor so that I didn't have to repeat the committer and the author's fields.

mentioned in merge request gitaly-proto!12 (merged)

@jacobvosmaer-gitlab would you agree to close this with the recommendation: "Keep messages specific to each endpoint. Try to keep them as flat as possible. Only add abstractions when they have a practical benefit"?

@eReGeBe that sounds good. Could you create a README that closes this issue?

assigned to @eReGeBe

mentioned in merge request gitaly-proto!14 (merged)

closed via merge request gitaly-proto!14 (merged)

mentioned in commit gitaly-proto@ee3423ac

mentioned in issue #318 (closed)