Codenotify: Notifying subscribers in CODENOTIFY files for diff bde3ece...be7329a.

Thanks for this!! Did you confirm that this issue isn't a problem for BitBucket Cloud, GitHub, or GitLab? My guess is that they'll probably all have the same or very similarly-behaving issues.

Adeola and I tested the other code hosts together in our 1:1, here are our findings:

• GitHub: It forks them successfully under unique names, with an increment (fork, fork-1).
• GitLab: It does not fork them successfully.
• BitBucket Cloud: It does not fork them successfully.

Even though GitHub handles the case okay, we agreed that our <org>-<repo> naming scheme is more informative than the base incrementing pattern that GitHub does by default. For consistency, we'll apply the same pattern across all code hosts when forking.

I will fix the remaining codehosts in a separate ticket and link it here

2 questions:

• Do we really want to step away from GitHubs default behavior? Wouldn't that be what customers would expect to happen for a fork?

• What protects us from the case when there's a repo named the same still? Previously, it would fail if you fork sourcegraph/src-cli into the user namespace if eseliger/src-cli existed, now it'll fail if eseliger/sourcegraph-src-cli exists. Also, for the standard case "repo doesn't conflict" we now create "unexpected" fork names.

Alternative idea:
What if we said If we want to fork:

• fetch from the code host if <namespace>/<reponame> already exists
• if not exists: Create
• if exists: Check if the found repo is already pointing to the correct parent repo
• if the parent: Go head and use that fork
• if not the parent: Create a new fork using the naming scheme proposed in this PR

(Just jotting down a bunch of passing thoughts, I defer to Adeola's judgment on what to do about this!)

Do we really want to step away from GitHubs default behavior? Wouldn't that be what customers would expect to happen for a fork?

I'd actually push back on that. I think it's a stretch to say most customers would expect something specific to happen when forking two repos with the same name, let alone expect what GitHub is doing, and currently it's non-deterministic which one gets forked first and gets the name user/fork vs. which one gets user/fork-1, which doesn't feel great either. I think if I were dealing with changesets across multiple code hosts, I'd rather see consistently-named forks (that are documented as diverging from GitHub's default behavior), so that I could easily tell which of my fork repos were generated from Sourcegraph.

What protects us from the case when there's a repo named the same still? Previously, it would fail if you fork sourcegraph/src-cli into the user namespace if eseliger/src-cli existed, now it'll fail if eseliger/sourcegraph-src-cli exists.

I agree this isn't ideal, but as you point out, this isn't a new problem, it's now just the same problem under a slightly different circumstance. It feels like that problem is adjacent to the one this PR is for, though. Not to say it wouldn't be great to get a 2-for-1, of course. 🙂

Also, for the standard case "repo doesn't conflict" we now create "unexpected" fork names.

"Unexpected" meaning just that generally it's expected that fork name == original name? I actually also don't think this would be a big problem for me so long as we document that we always name forks that way. It sure helps disambiguate things for generic, one-word repo names when the BitBucket UI doesn't tell you anything on the main page about what the repo was forked from (sigh). Perhaps we could even give more of a hint about this on the preview page when we show that little fork indicator on the changeset branch badge?

Alternative idea:
What if we said If we want to fork:

• fetch from the code host if <namespace>/<reponame> already exists
• if not exists: Create
• if exists: Check if the found repo is already pointing to the correct parent repo
• if the parent: Go head and use that fork
• if not the parent: Create a new fork using the naming scheme proposed in this PR

Wouldn't this also be confusing if only sometimes the fork repo shares the name with the original repo, and sometimes it doesn't? Especially if, again, the only determining factor for which is which is whoever got there first?

I also think it'd be hard to manage race conditions here, since all the forking stuff would be happening for multiple changesets in parallel. What if we're publishing changesets for both sourcegraph/src-cli and sourcegraph-testing/src-cli at the same time, but both checks find that courier-new/src-cli doesn't already exist? With this logic, they'd both try to create that fork themselves, so we'd be stuck back where we started where one fails on the naming collision. I suppose the retry mechanism would hopefully catch it the second time around and direct it to use the <project>-<repo> name pattern that time, but it feels messy if we let that happen. 😅

thanks @eseliger and @courier-new for your thoughts here.. really helpful. But yeah, I think I prefer to not use numbers in the names, as I do agree that as a user, I would have any expectation of what to expect in this case of forking repos with the same name, and I dont think that I would be shocked to see this name convention eseliger/sourcegraph-src-cli for example. I also remember us talking about possibly appending a number to the name in the past and I think we agreed that we didnt love that, so since I'd have no expecation as a user, I dont believe we need to do the number now just because github does it that way

Unfortunately when trying to update the test to ensure the fork is nil by adding:

fork, err := bbsSrc.GetUserFork(ctx, target)
assert.Nil(t, fork)

I'm now getting the following error: Error "getting username: Bitbucket API HTTP error: code=401 url=\"https://bitbucket.sgdev.org/rest/api/1.0/users?limit=1\" body=\"{\\\"errors\\\":[{\\\"context\\\":null,\\\"message\\\":\\\"Authentication failed.

I dont know how I've suddenly become unauthenticated, and I've taken a number of troubleshooting steps but im not longer able to update these tests.. when i run
curl -H "Authorization: Bearer $BITBUCKET_SERVER_TOKEN" https://bitbucket.sgdev.org/rest/api/1.0/users

I get data back, indicating this is not a token error.. I've spoken to @LawnGnome briefly about this and he isnt sure what can be causing this... @courier-new since I think you were previously authorized to do updates, do you mind helping me add those lines if you dont get an authorization error as well?

I'm happy to try re-recording the test for you, but I think that line can be safely added/modified/removed without affecting the golden test data, since we're not actually changing anything about the request to BitBucket! We have to re-record the tests when we change how bbsSrc.GetUserFork() is implemented, since that's where the request to the code host actually happens, but here we're only changing what we do with the values returned from the method.

(If that's enough to unblock you, we can save figuring out the authentication problem for while we're working on the docs!)

@courier-new yeah agreed, thanks! I do know that it isnt affecting the golden test data, however when I add that line and re run the rest, the test is now failing with that error when it wasnt before..so I think if i push the changes i'll have buildkite issues, or will i not? Im not sure what you mean by re-recording. I think unblocking me here would be adding the line im trying to add so I have the nil fork coverage, and can successfully merge this PR and then figuring out the auth problem as you suggested

Hmm, how are you trying to run the test when you get that failure? If you're running with -update, it'll try to actually talk to BitBucket again and overwrite the request + response it gets to those golden test data files. That's what I mean by re-record. 🙂 If you run it like a normal test without -update, it shouldn't talk to BitBucket at all, and it should only "replay" the interactions with the request + response it has saved. You shouldn't need to authenticate if you're running the tests in "replay" mode like that.

@courier-new when i remove the update flag im still getting the same error:

sources % go test -run TestBitbucketServerSource_GetUserFork

--- FAIL: TestBitbucketServerSource_GetUserFork (0.00s)
    --- FAIL: TestBitbucketServerSource_GetUserFork/not_a_fork (0.00s)
        bitbucketserver_test.go:773: 
                Error Trace:    /Users/adeola/Documents/sourcegraph/enterprise/internal/batches/sources/bitbucketserver_test.go:773
                Error:          Error "getting username: Bitbucket API HTTP error: code=401 url=\"https://bitbucket.sgdev.org/rest/api/1.0/users?limit=1\" body=\"{\\\"errors\\\":[{\\\"context\\\":null,\\\"message\\\":\\\"Authentication failed. Please check your credentials and try again.

can you add the lines? or do you think ill be fine if I just add the lines and push the code since they dont affect test data? Im a little scared too since its in a good state on buildkite now

Do you have a clean working tree beyond these 2 line changes? Is it possible you ran it once with -update previously and overwrote the response data file with the authentication error? Check and make sure you don't have any diffs for files in testdata first. If you do, discard them and try without -update one more time. If you don't... then I'll be really confused. 😬

@courier-new ohh thanks.. that did work, i still dont know why i have the auth error for when i actually do need to update tests, but thanks for pointing out that i can do this process without -update Ill be sure to add to the docs the importance of clearing response data..I think ill merge this now and get back to working with the other code hosts

Fix for Gitlab PR: #44458