Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

httpapi: Use repo.Name instead of URI for URL construction#51525

Merged
indradhanush merged 1 commit into
mainfrom
ig/bugfix-streaming-git-blame
May 5, 2023
Merged

httpapi: Use repo.Name instead of URI for URL construction#51525
indradhanush merged 1 commit into
mainfrom
ig/bugfix-streaming-git-blame

Conversation

@indradhanush

@indradhanush indradhanush commented May 5, 2023

Copy link
Copy Markdown
Contributor

Fixes https://github.com/sourcegraph/customer/issues/2113.

For repos synced with src server-git the URI is set to repos/<repo-name> in the database. This leads to an incorrect URL in the git-blames when the feature flag enable-streaming-git-blame is enabled.

Instead we should use the repo.Name because:

  1. For regular code hosts like GitHub.com, it is set to: github.com/sourcegraph/sourcegraph
  2. For src serve-git, it is set to sourcegraph (provided that's the repo being served)

Using the repo.Name in the URL construction fixes the issue.

Test plan

  • Tested locally
  • Added unit tests
  • Builds should pass

@indradhanush @jhchabran
httpapi: Use repo.Name insstead of URI for URL construction
31702ea 
Co-authored-by: Jean-Hadrien Chabran <jh@chabran.fr>
@cla-bot cla-bot Bot added the cla-signed label May 5, 2023
@indradhanush indradhanush requested review from a team and jhchabran May 5, 2023 15:51
@indradhanush indradhanush self-assigned this May 5, 2023
@indradhanush indradhanush added bug An error, flaw or fault that produces an incorrect or unexpected result, or behavior. customer Important issues reported or desired by a customer. labels May 5, 2023
@indradhanush indradhanush force-pushed the ig/bugfix-streaming-git-blame branch from 6a896c3 to 31702ea Compare May 5, 2023 15:53
@jhchabran

jhchabran commented May 5, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

@indradhanush you have a typo in the PR title and the commit :)

@indradhanush indradhanush changed the title httpapi: Use repo.Name insstead of URI for URL construction httpapi: Use repo.Name instead of URI for URL construction May 5, 2023
@indradhanush

indradhanush commented May 5, 2023

Copy link
Copy Markdown
Contributor Author

Good catch! Fixed.

@indradhanush indradhanush enabled auto-merge (squash) May 5, 2023 15:56
@indradhanush indradhanush merged commit f8128ce into main May 5, 2023
@indradhanush indradhanush deleted the ig/bugfix-streaming-git-blame branch May 5, 2023 16:01
keegancsmith
keegancsmith reviewed May 5, 2023
View reviewed changes

@keegancsmith keegancsmith left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I just audited all call sites to URI and this was the only misuse. We should consider removing this from the exported type to be honest.

In fact there was only one call site which read the value and luckily it did it correctly. Otherwise it was only used by code host syncers to write the value. Context: the only reason we store this value is we transparently look up by URI if looking up by Name fails to support the code host extensions correctly finding repos if sourcegraph is configured with a non-default repo name pattern.

Someone wanna send a PR which renames the field to URIDoNotUse or something? :D

@indradhanush

indradhanush commented May 5, 2023

Copy link
Copy Markdown
Contributor Author

@keegancsmith Thanks for the context. This looks like a nice good-first-issue. I'm pairing with @Rhia2 early next week and was in the hunt for something small and well scoped.

@Rhia2 Maybe we can take a look at this in that case if you're interested.

@DaedalusG

DaedalusG commented May 5, 2023

Copy link
Copy Markdown
Contributor

@indradhanush could we get this one backported so it lands in v5.0.4? Since its a bug fix?

@github-actions github-actions Bot mentioned this pull request May 8, 2023
Merged
github-actions Bot pushed a commit that referenced this pull request May 8, 2023
@indradhanush @jhchabran @github-actions
httpapi: Use repo.Name instead of URI for URL construction (#51525)
5c6ec91 
Fixes sourcegraph/customer#2113.

For repos synced with `src server-git` the URI is set to
`repos/<repo-name>` in the database. This leads to an incorrect URL in
the git-blames when the feature flag `enable-streaming-git-blame` is
enabled.

Instead we should use the `repo.Name` because:

1. For regular code hosts like GitHub.com, it is set to:
`github.com/sourcegraph/sourcegraph`
2. For `src serve-git`, it is set to `sourcegraph` (provided that's the
repo being served)

Co-authored-by: Jean-Hadrien Chabran <jh@chabran.fr>
(cherry picked from commit f8128ce)
@indradhanush

indradhanush commented May 8, 2023

Copy link
Copy Markdown
Contributor Author

@DaedalusG Yes, good point. Backporting now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@keegancsmith keegancsmith keegancsmith left review comments

@jhchabran jhchabran jhchabran approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@indradhanush indradhanush

Labels

bug An error, flaw or fault that produces an incorrect or unexpected result, or behavior. cla-signed customer Important issues reported or desired by a customer.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@indradhanush @jhchabran @DaedalusG @keegancsmith