Page MenuHomePhabricator

Allow code reviews across multiple repos
Closed, WontfixPublic

Description

It would be great if Differential would allow code reviews to span more than one repo. We use Mercurial but I guess it would be nice to have it supported with other source control systems as well.

Event Timeline

nikolay.metchev raised the priority of this task from to Needs Triage.
nikolay.metchev updated the task description. (Show Details)
nikolay.metchev added a project: Differential.
nikolay.metchev added a subscriber: nikolay.metchev.

At the moment we have multiple repos in Mercurial that each contain one logical module of code. When we produce a binary we take build artifacts from multiple repos as dependencies to produce the final binary.
For example we have a repo called common which contains code that is common across all other repos.
As such it is possible that a bug fix or new feature will sometimes involve changing code in multiple repos.
As it stands we need to create multiple code review requests (one per repo) even though there is only one actual change.

From a high level I can see benefit to having this feature:

  1. Changes which span multiple repositories would lend to discussions in a review spanning across multiple revisions.
  2. From a reporting/auditing standpoint having a single revision to refer to instead of multiple.

However, I can see how mixing multiple repositories would be difficult to define. Creating, updating, landing diffs would need to manage each repository involved - if one fails/conflicts then what should happen with the others, etc. It sounds like this could be painful with regards to automation.

I think 'Edit Dependencies' works well enough for our situation.


We have multiple repositories which are inter-dependent, for example:

  • main-product
    • secret-impl
    • test-data

We keep the secret-impl repo separate with restricted access, so not all engineers can see it (trade secrets, proprietary implementations, etc.). When changes are made there, a jar is built, obfuscated, and copied to the main-product repo so it can be used.

The test-data repo is used for storing large/verified datasets used by automated test-cases which are defined in main-product.

From the nature of these repos, changes in one of the 'child' repos almost always requires changes in the 'parent' repo. Sometimes the dependency is reversed, or they both rely on changes in the other.

If this makes sense, it's probably related to T9287.


One of my projects is an Android derivative, which is using Gerrit and Google's repo program to manage a huge project into many git repositories; Naturally, some changes span multiple repositories, and landing one of the changes doesn't make sense.
As far as I can see, the way Gerrit/repo handle this is that the "repo upload" command creates a bunch of review objects, named "My change (n/k)", where k is the total number of changes in the batch. Each review object is limited to a single git repo, and there's no explicit link between the review objects.

At other times, when I had a multi-repository-spanning change (Using Phabricator), I'd usually just make one of the revisions Depend on the the other, in the order at which they'd need to be landed/released. That was a while back though, and I don't know how arc-patch will react to that now.

eadler added a project: Restricted Project.Jul 7 2016, 5:10 PM

How would this work in CI? We use jenkins to test differentials. Herald starts a jenkins-job when a differential connected to a spesific repository is created or edited. Heralds sends the repository URL and commit id of current HEAD in differential to jenkins, and this then runs tests on HEAD of differential using

arc patch --nobranch --no-ansi --diff $DIFF_ID --nocommit

How would we be able to seperate the affected code and from what repositories it's from?

We will almost certainly never support diffs coming from multiple different repository sources. They can't be built, can't be updated, can't be patched, can't be meaningfully interdiffed, local commits are meaningless, can't be atomically landed, and they basically don't make sense in general.

Any version of this that is eventually built in the upstream will look more like a way to collect several revisions together and mark them as closely related, while each revision continues to function very similarly to how revisions function today.

You can already do this with "Depends On", and I'd ideally like that to cover this use case as well. Historically, "Depends On" was pretty buried, but we've made several recent changes to give it more prominence and have plans to continue in this direction.

epriestley claimed this task.

I'm going to close this as non-actionable since it doesn't clearly describe a use case, there's no current customer interest, and it's not clear what problems we'd like to solve that aren't already solved by a stronger "Depends On". The "Depends On" relationship has also become stronger and better integrated since this was originally filed, and it's possible it addresses some or all of the original use cases.

Broadly, I'm onboard with grouping related revisions together, but I'd be surprised if we ever ship some kind of giant merged mega-revision since it creates a huge number of problems, detailed above.