the graphqlbackend package is quite a behemoth already. I recommend new code which contains business logic to go somewhere else and then the resolver code is very very lightweight. Can you update this PR to put the P4 parsing coding and the other non-graphql specific p4 code somewhere else?

It also feels a bit weird to update just this graphql resolver to massage the subject. But I don't have a better suggestion, so I guess lets do this. But it almost feels like in some other layer you want to translate git commit messages into a different format. To me it feels like that should just be done in the p4-fusion layer? IE all you are doing is translating the message here, why not just write the mssage you want in p4-fusion? That does sound a bit more hectic to do though.

Both git p4 and p4-fusion add Perforce information to the git commit message when they convert Perforce changelists to git commits. That information has traditionally simply been displayed to the user in the UI, but now we want to hide that information and show the user only the original commit message from Perforce. However, the extra information is important because it contains the Changelist Id, which we want to display in the UI instead of commit hashes, and also use in URLs as we go forward with these changes. We are hesitant to change p4-fusion and git p4 (we don't own those tools, we currently need that changelist info, and there are clients with already-synced depots, so they have this commit message format), so we need to parse the commit message (and subject, for ones generated by p4-fusion) at some point along the way to the UI; there might be a better place to do it, but I'm not sure we can push it any deeper than this.

Ok I agree with that point. I still think p4 specific business logic like this should not live inside the graphql resolver for a git commit subject. It feels alright that we inject something here though that looks like this. IE I am mostly taking issue with strings.HasPrefix(commit.Message.Body(), "[p4-fusion") { living here. It feels like the impact on this function should be mostly a single function call which works something like

if subject, ok = maybeTransformP4Subject(ctx, r.repoResolver, commit.Message); ok {
  return subject
}

Or alternatively based on slack discussions the design of this is gonna change a bit and the impl of subject resolver will live in something p4 specific.

Yep, pushing that code you mentioned into a function would be prettier and more portable.

I remember this being something @mrnugget was worried about happening (similarly to adding additional isX isY stuff to the Repository and other types)

One idea we were thinking of having as an adopted pattern in the packages side of things was moving to union types across the graphql API, but thatd be a much bigger endeavor (for which we had a staffing proposal under the packages project, unfortunately not going ahead this quarter :/ )

I agree with this concern. Should probably use the same suggestion I used here https://github.com/sourcegraph/sourcegraph/pull/51291#discussion_r1182834582

Is your suggestion something along the lines of my second suggestion? IE rely on ... on PerforceRepo {

Is your suggestion something along the lines of my second suggestion? IE rely on ... on PerforceRepo {

I think along that line of thought, yea. In the client code you can switch on __typename and render differently (or different components entirely) based on that value to e.g. include or exclude stuff based on if its e.g. a PerforceRepo, PackageRepo, GitRepo etc

That was originally the idea we had come up with in a chat between Philipp Spiess, Ryan Slade, Peter and myself. I had planned for it to be a subproject under this quarters packages project and for the idea to be picked up then by other non-git ecosystems, but alas no staffing.

In our case, this might not be a trivial change though, since you'd probably need to change the "main case" (git repository) in all providers. But maybe there's a way around that and make it gradual, i.e. define a interface PerforceDepot and then implement ToPerforceDepot() on the type you have here and return new type when it's a perforce depot.

My only concern is if this is the right time to be implementing this as an interface, since at the root of it all its still a git repo that we're presenting as a perforce depot by only tweaking the commit message and the commit ref.

It would have made sense if we were to introduce perforce depots as first class citizens and then this change might have been a better time to do it then. What do you think?

I think it's the same thing as what you already have: an enum and a field that's only valid if the enum has a certain value. Except that the interfaces hide that from you and are more GraphQL-native. With your solution you'd need to query this:

query {
  repo(name: "foo/bar") {
    # [...]
    sourceType
    perforceChangelist
  }
}

Then in your client-side application code you'd have to do this:

if (repo.sourceType === sourceType.PERFORCE_DEPOT) {
  // use changelist
}

With an interface you'd do this (I think, again, you'd need to confirm this):

query {
  repo(name: "foo/bar") {
    # [...]
    ... on PerforceDepot {
      perforceChangelist
    }
  }
}

And you'd get a PerforceDepot type in your client-side code.

What I'm unsure about is whether repo needs to be an interface for that or not. If it has to be an interface, then it's a lot of work to change, I think.

Okay - let me give this a try and come back with what I find out.

Yes, I think in order to do that it would have to become an interface. I think further down the line it would be very nice to have a interface Repository and then two types type Depot and type GitRepository that will host non-common fields to have a proper abstraction. I don't know when we want to go more first class here, as long as it's only the 2 or 3 fields here that are depot specific, I think we can live with that for now, but we should account for a more complex migration of the API further down the line.

Summary based on quick chat on Slack

Here's the ideal scenario:

1. Define a new interface (say Source) with the common fields between a perforce depot and a git repo
2. Update repo to implement this Source interface and add fields like stars , fork etc
3. Create a new type that also implements Source and adds the perforceChangelist on top of it

But this is likely going to be a big change (especially in the client) and out of scope of this PR but should be on our minds to get around to eventually.

For the time being - the approach of enum should be good to go ahead with.

Ok I agree with that point. I still think p4 specific business logic like this should not live inside the graphql resolver for a git commit subject. It feels alright that we inject something here though that looks like this. IE I am mostly taking issue with strings.HasPrefix(commit.Message.Body(), "[p4-fusion") { living here. It feels like the impact on this function should be mostly a single function call which works something like

if subject, ok = maybeTransformP4Subject(ctx, r.repoResolver, commit.Message); ok {
  return subject
}

Or alternatively based on slack discussions the design of this is gonna change a bit and the impl of subject resolver will live in something p4 specific.

use the returned repo above, don't directly access innerRepo