batches: fix publishing changesets to forks (sourcegraph#43214)

Fixes the way we procure a clone URL for a repo when pushing commits and publishing changesets to a fork

client/web/src/enterprise/batches/preview/list/VisibleChangesetApplyPreviewNode.story.tsx

@@ -80,7 +80,7 @@ ChangeDiff.args = { node: stories['Change diff'] }
 ChangeDiff.storyName = 'Change diff'
 
 export const CloseChangeset = Template.bind({})
-CloseChangeset.args = { node: stories['Change diff'] }
+CloseChangeset.args = { node: stories['Close changeset'] }
 CloseChangeset.storyName = 'Close changeset'
 
 export const DetachChangeset = Template.bind({})
@@ -103,6 +103,6 @@ export const ForkedRepo = Template.bind({})
 ForkedRepo.args = { node: stories['Forked repo'] }
 ForkedRepo.storyName = 'Forked repo'
 
-export const ReattachedChangeset = Template.bind({})
-ForkedRepo.args = { node: stories['Reattach changeset'] }
-ForkedRepo.storyName = 'Reattach Changeseet'
+export const ReattachChangeset = Template.bind({})
+ReattachChangeset.args = { node: stories['Reattach changeset'] }
+ReattachChangeset.storyName = 'Reattach Changeset'

client/web/src/enterprise/batches/preview/list/storyData.ts

@@ -381,39 +381,64 @@ export const visibleChangesetApplyPreviewNodeStories = (
  },
  },
  },
- 'Close changeset': {
+ 'Change diff': {
  __typename: 'VisibleChangesetApplyPreview',
- operations: [ChangesetSpecOperation.CLOSE, ChangesetSpecOperation.DETACH],
+ operations: [ChangesetSpecOperation.UPDATE],
  delta: {
  titleChanged: false,
  baseRefChanged: false,
- diffChanged: false,
+ diffChanged: true,
  bodyChanged: false,
  authorEmailChanged: false,
  authorNameChanged: false,
  commitMessageChanged: false,
  },
  targets: {
- __typename: 'VisibleApplyPreviewTargetsDetach',
+ __typename: 'VisibleApplyPreviewTargetsUpdate',
+ changesetSpec: baseChangesetSpec(9, publicationStateSet ? true : null),
  changeset: {
  id: '123123',
- title: 'Le open changeset',
+ title: 'Change diff',
  state: ChangesetState.OPEN,
- repository: testRepo,
  externalID: '123',
  externalURL: {
  url: 'https://test.test/123',
  },
- diffStat: {
- added: 10,
- deleted: 18,
+ currentSpec: {
+ description: {
+ __typename: 'GitBranchChangesetDescription',
+ baseRef: 'master',
+ body: 'body',
+ commits: [
+ {
+ subject: 'Abc',
+ body: null,
+ author: {
+ avatarURL: null,
+ displayName: 'alice',
+ email: '[email protected]',
+ user: null,
+ },
+ },
+ ],
+ title: 'Title',
+ },
+ },
+ author: {
+ displayName: 'Alice',
+ email: '[email protected]',
+ user: {
+ displayName: 'Alice',
+ url: '/users/alice',
+ username: 'alice',
+ },
  },
  },
  },
  },
- 'Detach changeset': {
+ 'Close changeset': {
  __typename: 'VisibleChangesetApplyPreview',
- operations: [ChangesetSpecOperation.DETACH],
+ operations: [ChangesetSpecOperation.CLOSE, ChangesetSpecOperation.DETACH],
  delta: {
  titleChanged: false,
  baseRefChanged: false,
@@ -441,76 +466,51 @@ export const visibleChangesetApplyPreviewNodeStories = (
  },
  },
  },
- 'Change base ref': {
+ 'Detach changeset': {
  __typename: 'VisibleChangesetApplyPreview',
- operations: [ChangesetSpecOperation.UPDATE],
+ operations: [ChangesetSpecOperation.DETACH],
  delta: {
  titleChanged: false,
- baseRefChanged: true,
+ baseRefChanged: false,
  diffChanged: false,
  bodyChanged: false,
  authorEmailChanged: false,
  authorNameChanged: false,
  commitMessageChanged: false,
  },
  targets: {
- __typename: 'VisibleApplyPreviewTargetsUpdate',
- changesetSpec: baseChangesetSpec(8, publicationStateSet ? true : null),
+ __typename: 'VisibleApplyPreviewTargetsDetach',
  changeset: {
  id: '123123',
- title: 'Change base ref',
+ title: 'Le open changeset',
  state: ChangesetState.OPEN,
+ repository: testRepo,
  externalID: '123',
  externalURL: {
  url: 'https://test.test/123',
  },
- currentSpec: {
- description: {
- __typename: 'GitBranchChangesetDescription',
- baseRef: 'main',
- body: 'body',
- commits: [
- {
- subject: 'Abc',
- body: null,
- author: {
- avatarURL: null,
- displayName: 'alice',
- email: '[email protected]',
- user: null,
- },
- },
- ],
- title: 'Title',
- },
- },
- author: {
- displayName: 'Alice',
- email: '[email protected]',
- user: {
- displayName: 'Alice',
- url: '/users/alice',
- username: 'alice',
- },
+ diffStat: {
+ added: 10,
+ deleted: 18,
  },
  },
  },
  },
- 'Change diff': {
+ 'Change base ref': {
  __typename: 'VisibleChangesetApplyPreview',
  operations: [ChangesetSpecOperation.UPDATE],
  delta: {
  titleChanged: false,
- baseRefChanged: false,
- diffChanged: true,
+ baseRefChanged: true,
+ diffChanged: false,
  bodyChanged: false,
  authorEmailChanged: false,
  authorNameChanged: false,
  commitMessageChanged: false,
  },
  targets: {
  __typename: 'VisibleApplyPreviewTargetsUpdate',
- changesetSpec: baseChangesetSpec(9, publicationStateSet ? true : null),
+ changesetSpec: baseChangesetSpec(8, publicationStateSet ? true : null),
  changeset: {
  id: '123123',
  title: 'Change base ref',
@@ -522,7 +522,7 @@ export const visibleChangesetApplyPreviewNodeStories = (
  currentSpec: {
  description: {
  __typename: 'GitBranchChangesetDescription',
- baseRef: 'master',
+ baseRef: 'main',
  body: 'body',
  commits: [
  {
@@ -676,7 +676,7 @@ export const visibleChangesetApplyPreviewNodeStories = (
  targets: {
  __typename: 'VisibleApplyPreviewTargetsAttach',
  changesetSpec: baseChangesetSpec(12, publicationStateSet ? true : null, {
- forkTarget: { pushUser: true, namespace: null },
+ forkTarget: { pushUser: true, namespace: '<user>' },
  description: {
  __typename: 'GitBranchChangesetDescription',
  baseRepository: testRepo,

enterprise/cmd/frontend/internal/batches/resolvers/changeset_spec.go

@@ -234,5 +234,9 @@ func (r *forkTargetResolver) PushUser() bool {
 }
 
 func (r *forkTargetResolver) Namespace() *string {
- return r.changesetSpec.GetForkNamespace()
+ // We don't use `changesetSpec.GetForkNamespace()` here because it returns `nil` if
+ // the namespace matches the user default namespace. This is a perfectly reasonable
+ // thing to do for the way we use the method internally, but for resolving this field
+ // on the GraphQL scehma, we want to return the namespace regardless of what it is.
+ return r.changesetSpec.ForkNamespace
 }

enterprise/internal/batches/reconciler/executor.go

@@ -168,8 +168,6 @@ func (e *executor) pushChangesetPatch(ctx context.Context) (err error) {
 
  // Create a commit and push it
  // Figure out which authenticator we should use to modify the changeset.
- // au is nil if we want to use the global credentials stored in the external
- // service configuration.
  css, err := e.changesetSource(ctx)
  if err != nil {
  return err

enterprise/internal/batches/sources/bitbucketcloud.go

@@ -214,23 +214,23 @@ func (s BitbucketCloudSource) MergeChangeset(ctx context.Context, cs *Changeset,
 // the given namespace, ensuring that the fork exists and is a fork of the
 // target repo.
 func (s BitbucketCloudSource) GetNamespaceFork(ctx context.Context, targetRepo *types.Repo, namespace string) (*types.Repo, error) {
- upstreamRepo := targetRepo.Metadata.(*bitbucketcloud.Repo)
+ targetMeta := targetRepo.Metadata.(*bitbucketcloud.Repo)
 
  // Figure out if we already have the repo.
- if fork, err := s.client.Repo(ctx, namespace, upstreamRepo.Slug); err == nil {
- return s.createRemoteRepo(targetRepo, fork), nil
+ if fork, err := s.client.Repo(ctx, namespace, targetMeta.Slug); err == nil {
+ return s.copyRepoAsFork(targetRepo, fork)
  } else if !errcode.IsNotFound(err) {
  return nil, errors.Wrap(err, "checking for fork existence")
  }
 
- fork, err := s.client.ForkRepository(ctx, upstreamRepo, bitbucketcloud.ForkInput{
+ fork, err := s.client.ForkRepository(ctx, targetMeta, bitbucketcloud.ForkInput{
  Workspace: bitbucketcloud.ForkInputWorkspace(namespace),
  })
  if err != nil {
  return nil, errors.Wrap(err, "forking repository")
  }
 
- return s.createRemoteRepo(targetRepo, fork), nil
+ return s.copyRepoAsFork(targetRepo, fork)
 }
 
 // GetUserFork returns a repo pointing to a fork of the given repo in the
@@ -244,14 +244,17 @@ func (s BitbucketCloudSource) GetUserFork(ctx context.Context, targetRepo *types
  return s.GetNamespaceFork(ctx, targetRepo, user.Username)
 }
 
-func (BitbucketCloudSource) createRemoteRepo(targetRepo *types.Repo, fork *bitbucketcloud.Repo) *types.Repo {
- // This needs to be good enough to get the right values out of
- // bitbucketCloudCloneURL(), which only looks at the metadata, so we can
- // just copy it in over the top of the targetRepo.
- remoteRepo := *targetRepo
- remoteRepo.Metadata = fork
+func (s BitbucketCloudSource) copyRepoAsFork(targetRepo *types.Repo, fork *bitbucketcloud.Repo) (*types.Repo, error) {
+ targetMeta := targetRepo.Metadata.(*bitbucketcloud.Repo)
 
- return &remoteRepo
+ // Now we make a copy of the target repo, but with its sources and metadata updated to
+ // point to the fork
+ forkRepo, err := CopyRepoAsFork(targetRepo, fork, targetMeta.FullName, fork.FullName)
+ if err != nil {
+ return nil, errors.Wrap(err, "updating target repo sources")
+ }
+
+ return forkRepo, nil
 }
 
 func (s BitbucketCloudSource) annotatePullRequest(ctx context.Context, repo *bitbucketcloud.Repo, pr *bitbucketcloud.PullRequest) (*bbcs.AnnotatedPullRequest, error) {

enterprise/internal/batches/sources/bitbucketcloud_test.go

@@ -572,7 +572,13 @@ func TestBitbucketCloudSource_Fork(t *testing.T) {
  FullName: "upstream/repo",
  Slug: "repo",
  }
- upstreamRepo := &types.Repo{Metadata: upstream}
+ urn := extsvc.URN(extsvc.KindBitbucketCloud, 1)
+ upstreamRepo := &types.Repo{Metadata: upstream, Sources: map[string]*types.SourceInfo{
+ urn: {
+ ID: urn,
+ CloneURL: "https://bitbucket.org/upstream/repo",
+ },
+ }}
 
  fork := &bitbucketcloud.Repo{
  UUID: "fork-uuid",
@@ -606,10 +612,12 @@ func TestBitbucketCloudSource_Fork(t *testing.T) {
  return fork, nil
  })
 
- repo, err := s.GetNamespaceFork(ctx, upstreamRepo, "fork")
+ forkRepo, err := s.GetNamespaceFork(ctx, upstreamRepo, "fork")
  assert.Nil(t, err)
- assert.NotNil(t, repo)
- assert.Same(t, fork, repo.Metadata)
+ assert.NotNil(t, forkRepo)
+ assert.NotEqual(t, forkRepo, upstreamRepo)
+ assert.Equal(t, fork, forkRepo.Metadata)
+ assert.Equal(t, forkRepo.Sources[urn].CloneURL, "https://bitbucket.org/fork/repo")
  })
 
  t.Run("fork error", func(t *testing.T) {
@@ -649,10 +657,12 @@ func TestBitbucketCloudSource_Fork(t *testing.T) {
  return fork, nil
  })
 
- repo, err := s.GetNamespaceFork(ctx, upstreamRepo, "fork")
+ forkRepo, err := s.GetNamespaceFork(ctx, upstreamRepo, "fork")
  assert.Nil(t, err)
- assert.NotNil(t, repo)
- assert.Same(t, fork, repo.Metadata)
+ assert.NotNil(t, forkRepo)
+ assert.NotEqual(t, forkRepo, upstreamRepo)
+ assert.Equal(t, fork, forkRepo.Metadata)
+ assert.Equal(t, forkRepo.Sources[urn].CloneURL, "https://bitbucket.org/fork/repo")
  })
  })
 

enterprise/internal/batches/sources/bitbucketserver.go

@@ -310,6 +310,8 @@ func (s BitbucketServerSource) callAndRetryIfOutdated(ctx context.Context, c *Ch
  return newestPR, nil
 }
 
+// GetUserFork returns a repo pointing to a fork of the given repo in the
+// currently authenticated user's namespace.
 func (s BitbucketServerSource) GetUserFork(ctx context.Context, targetRepo *types.Repo) (*types.Repo, error) {
  parent := targetRepo.Metadata.(*bitbucketserver.Repo)
 
@@ -319,9 +321,12 @@ func (s BitbucketServerSource) GetUserFork(ctx context.Context, targetRepo *type
  return nil, errors.Wrap(err, "getting username")
  }
 
- // See if we already have a fork. We have to prepend a tilde to the user
- // name to make this a "user-centric URL" in Bitbucket Server parlance.
- fork, err := s.getFork(ctx, parent, "~"+user)
+ // We have to prepend a tilde to the user name to make this a "user-centric URL" in
+ // Bitbucket Server parlance.
+ forkNamespace := "~" + user
+
+ // See if we already have a fork.
+ fork, err := s.getFork(ctx, parent, forkNamespace)
  if err != nil && !bitbucketserver.IsNotFound(err) {
  return nil, errors.Wrapf(err, "getting user fork for %q", user)
  }
@@ -334,9 +339,12 @@ func (s BitbucketServerSource) GetUserFork(ctx context.Context, targetRepo *type
  }
  }
 
- return createRemoteRepo(targetRepo, fork), nil
+ return s.copyRepoAsFork(targetRepo, fork, forkNamespace)
 }
 
+// GetNamespaceFork returns a repo pointing to a fork of the given repo in
+// the given namespace, ensuring that the fork exists and is a fork of the
+// target repo.
 func (s BitbucketServerSource) GetNamespaceFork(ctx context.Context, targetRepo *types.Repo, namespace string) (*types.Repo, error) {
  parent := targetRepo.Metadata.(*bitbucketserver.Repo)
 
@@ -356,18 +364,23 @@ func (s BitbucketServerSource) GetNamespaceFork(ctx context.Context, targetRepo
  }
  }
 
- return createRemoteRepo(targetRepo, fork), nil
+ return s.copyRepoAsFork(targetRepo, fork, namespace)
 }
 
-func createRemoteRepo(targetRepo *types.Repo, fork *bitbucketserver.Repo) *types.Repo {
- // We have to make a legitimate seeming *types.Repo.
- // bitbucketServerCloneURL() ultimately only looks at the
- // bitbucketserver.Repo in the Metadata field, so we'll replace that with
- // the fork's metadata, and all should be well.
- remoteRepo := *targetRepo
- remoteRepo.Metadata = fork
+func (s BitbucketServerSource) copyRepoAsFork(targetRepo *types.Repo, fork *bitbucketserver.Repo, forkNamespace string) (*types.Repo, error) {
+ targetMeta := targetRepo.Metadata.(*bitbucketserver.Repo)
+
+ targetNameAndNamespace := targetMeta.Project.Key + "/" + targetMeta.Slug
+ forkNameAndNamespace := forkNamespace + "/" + targetMeta.Slug
+
+ // Now we make a copy of the target repo, but with its sources and metadata updated to
+ // point to the fork
+ forkRepo, err := CopyRepoAsFork(targetRepo, fork, targetNameAndNamespace, forkNameAndNamespace)
+ if err != nil {
+ return nil, errors.Wrap(err, "updating target repo sources")
+ }
 
- return &remoteRepo
+ return forkRepo, nil
 }
 
 var (