Periods in repository shortnames make the repo unreachable
Closed, ResolvedPublic

Description

Reproduction Steps:

  • Click Create RepositoryCreate Git Repository in Diffusion
  • Give the repository a shortname with a period (e.g., test.test)
  • Create Repository

Expected Result:

  • I be taken to https://phabricator.domain.com/source/repo.shortname/manage to edit additional settings and activate the repository

Observed Result:

  • 404 error

I noticed this locally after a user reported the issue over the weekend. We're running

rP74a0e5cd7930ac06013ed6fafc4fd96e7730b40c (Sat, Nov 19)
rARCe17fe43ca3fe6dc6dd0b5ce056f56310ea1d3d51 (Oct 21 2016)
rPHUb4f866bd75ec138397a16337bc5d326e586a6276 (Fri, Nov 18)

I also reproduced this with the above steps on a fresh Phacility instance.

I didn't see this reported anywhere, so apologies if it's a dupe!

jcox added a subscriber: jcox.Nov 21 2016, 8:02 PM

Confirmed this on my local. It seems to be due to D16851, specifically this line: https://secure.phabricator.com/source/phabricator/browse/master/src/applications/diffusion/application/PhabricatorDiffusionApplication.php$104.

Removing the period from the negated regex group fixed it for me, although I'm not sure that is the right solution or if we'd want to enforce no periods on creation.

This is because we now recognize shortname.git in some cases, and it's ambiguous if quack.git is "the repository with shortname quack.git", or "the repository with shortname quack, addressed as a Git repository".

I'm leaning toward jut forbidding periods. These don't seem to be a strong precedent for using periods in GitHub repository names (e.g., node.js is just nodejs), and this generally seems like sort of a bad idea?

Alternatively, we could forbid just .git at the end. This is approximately what GitHub does. But we may have to forbid other stuff down the road.

How saddened would you be by a "no dots in short names" rule?

(From our own install's perspective) I don't think the would make anyone at all sad. It seemed to be a pretty random decision by one of our newer users while he was playing around with things.

Okay, let me ban them for now and if it causes too much of an issue we can try just banning .git at the end.

I should probably have anticipated this but missed it when selecting the range of valid characters.

Oh, hrrrm, this is actually more involved since it's not just the .git clone URI that's in trouble. So I can't just ban . without fixing existing short names.

I'll just ban .git instead and hope no one named a repository quack.git since this feature hasn't been available or useful for too long. <_< >_>

chad renamed this task from Periods in repository shortnames make the repo unreachable. to Periods in repository shortnames make the repo unreachable.Nov 21 2016, 11:45 PM

I think this should be fixed now. Note that we still have T11165 floating around which may make it look like this is still an issue, by sending you to the wrong place when you hit "Save". That bug is dumb but fairly harmless so I haven't chased it down yet.

No problem, thanks for the report!