Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

Update brew tap to work without 'homebrew-' #18366

Closed
wants to merge 1 commit into from
Closed

Update brew tap to work without 'homebrew-' #18366

wants to merge 1 commit into from

Conversation

telemachus
Copy link
Contributor

Currently brew tap only works on repos with 'homebrew-' in their name.

This version tries the repo name as is and then falls back to try
'homebrew-repo' only if that fails.

I've also tweaked the regex in tap_args to allow '-' in repo names. The
previous regex required a match on \w. This made it impossible for people
to tap repos with names like 'username/why-not'.

Ping @MikeMcQuaid (screwed up and opened a new pr again - too tired, sorry)

@MikeMcQuaid
Copy link
Member

Looks great. Will give it a bit for anyone else to chime in and I'll merge shortly if not.

@telemachus
Copy link
Contributor Author

Thanks and sounds good. Once this is merged, I'll update the brew tap wiki page so that it's up to date with the changes.

@englishm
Copy link
Contributor

englishm commented Mar 9, 2013

👍

@telemachus
Copy link
Contributor Author

@MikeMcQuaid ping

@MikeMcQuaid
Copy link
Member

Sorry, was on holiday. This is on my todo list; will get to it shortly!

@telemachus
Copy link
Contributor Author

@MikeMcQuaid No worries. I just wanted to bump it in case it was forgotten. (Hope the holiday was good.)

@MikeMcQuaid
Copy link
Member

Pushed with tweaks. Thanks!

MikeMcQuaid added a commit that referenced this pull request Mar 12, 2013
@MikeMcQuaid MikeMcQuaid reopened this Mar 12, 2013
@MikeMcQuaid
Copy link
Member

Sorry, reverted this.

@MikeMcQuaid
Copy link
Member

See #18432.

@telemachus
Copy link
Contributor Author

@MikeMcQuaid Argh. So two quick questions:

  1. Would you consider some other complicated thing to make it possible to tap things without 'homebrew-' in their name? Or do you think it's just not worth it?
  2. What if I just resubmit the change to the regex that allows you to create taps with (extra) hyphens in their name?

Damn, damn, damn.

@MikeMcQuaid
Copy link
Member

@telemachus Chill, don't worry about it. I didn't see this either because I also had the authentication cached locally. I think doing something with Curl to check the remote first makes sense to me.

@telemachus
Copy link
Contributor Author

Cool. I won't get right back to this, but I'll play with the curl idea next. Now that I think of it, if we do the curl check up front, then we can (maybe) remove the later begin curl...rescue block later in the method. That might make life simpler all around.

@ghost
Copy link

ghost commented Mar 12, 2013

You can use github api: http://developer.github.com/v3/repos. But that still wouldn't help you discover private repos.

@Sharpie
Copy link
Contributor

Sharpie commented Mar 12, 2013

@telemachus

I think option 1 is going to be a tough sell. Why should we implement, debug and support complicated things for the underwhelming payoff of saving people from typing homebrew- once or twice when they set up a new tap? I agree that being locked to a specific prefix is not ideal, but the prefix does have some value as a sort of namespace among the universe of GitHub repos.

Option 2 is completely reasonable and something we should implement.

@Sharpie
Copy link
Contributor

Sharpie commented Mar 12, 2013

You can use github api: http://developer.github.com/v3/repos/contents/#get-contents it is GET and no authentication necessary.

That appears to return "not found" for private repositories. Many people using taps are using private repos to keep custom stuff private----we cannot assume "not found" means "not tappable".

@Sharpie
Copy link
Contributor

Sharpie commented Mar 12, 2013

Case in point:

➜  ~  curl -i https://api.github.com/repos/adamv/homebrew-alt/contents
HTTP/1.1 404 Not Found
Server: GitHub.com
Date: Tue, 12 Mar 2013 19:13:22 GMT
Content-Type: application/json; charset=utf-8
Connection: keep-alive
Status: 404 Not Found
X-RateLimit-Limit: 60
X-RateLimit-Remaining: 57
X-GitHub-Media-Type: github.beta
X-Content-Type-Options: nosniff
Content-Length: 29

{
  "message": "Not Found"
}

➜  ~  brew tap adamv/alt
Username for 'https://github.com': Sharpie
Password for 'https://Sharpie@github.com': 

Cloning into '/usr/local/Library/Taps/adamv-alt'...
Warning: Could not tap adamv/alt/ninja over mxcl/master/ninja
Warning: Could not tap adamv/alt/pbrt over mxcl/master/pbrt
Tapped 32 formula

It looks like you tapped a private repository
In order to not input your credentials every time
you can use git HTTP credential caching or issue the
following command:

   cd /usr/local/Library/Taps/adamv-alt
   git remote set-url origin git@github.com:adamv/homebrew-alt.git

@englishm
Copy link
Contributor

Separate potential issue I just realized:

There's a possibility that a username/reponame repo could shadow an existing username/homebrew-reponame repo for new taps. Checking username/homebrew-reponame first would be more backwards compatible.

@Sharpie I disagree about the value of this feature. I think it's definitely worth the effort to allow repos without the "homebrew-" prefix in the name. (see: silentbicycle/tangram#1 (comment) for an example)

@ghost
Copy link

ghost commented Mar 12, 2013

What about supporting non github taps ?

@englishm
Copy link
Contributor

@humdedum that is also desired, but there are unresolved questions about where to place the taps on a user's local filesystem to avoid conflicts. See #11695.

@MikeMcQuaid
Copy link
Member

The simplest solution given we want to support non-github taps and to avoid shadowing or extra calls is I think to just allow using non-homebrew-prefixed taps by specifying the full Git URL.

@MikeMcQuaid
Copy link
Member

(Ok, so my "simplest solution" isn't very simple. Sue me!)

@ghost
Copy link

ghost commented Mar 12, 2013

Or would it be unrecoverable cool factor loss if we require full user/repo and ditch the shortcut ?

@telemachus
Copy link
Contributor Author

@Sharpie I have a version that checks for repos at the low, low cost of (at worst) two extra curl calls. I'll be putting it up shortly. I really don't think that it should be that big a deal. The code is not especially complicated (it's a handful of lines), and the time factor is still negligible.

It also has the benefit of fixing the bug that @jacknagel mentions in existing versions of brew tap: namely that if you have a typo when you try to tap something, you get bugged for credentials. If you run the curl checks first, this will never happen. In any case, I'm cleaning it up this morning and will post it shortly today.

Currently `brew tap` only works on repos with 'homebrew-' in their name.

This version tries to find 'homebrew-repo' and then falls back to try
'repo' if that fails.

In the process of fixing #18432, I've made a number of changes:

+ Followed @englishm's advice and checked for 'homebrew-repo' *first*.
+ Rather than `begin...rescue` for curl, I've used backticks and a manual
  check of $? to be more consistent with the rest of the method.
+ Fixed the error message if the repo is private: the read-only git URL
  should be in the form git://github.com/#{repouser}/#{repo}.git I think.

I've also tweaked the regex  in tap_args to allow '-' in repo names. The
previous regex required a match on \w. This made it impossible for people
to tap repos with names like 'username/why-not'.
@telemachus
Copy link
Contributor Author

Ping @MikeMcQuaid @Sharpie @jacknagel @englishm

I think this version covers the bases without too much extra code.

  • It allows people to tap repos like username/foo as well as username/foo-bar.
  • It still allows username/foo as shorthand for username/homebrew-foo. And the
    homebrew- version is checked first as @englishm suggested.
  • It fixes the bug in earlier versions where a typo will cause git to ask you for credentials.
  • It fixes my bug which led to git asking you for credentials.
  • It also fixes the error message about private repos, which I think printed
    the wrong URL for read-only git repos. (The old version suggests ssh URLs
    which are not read-only and would prompt for your ssh key, I think.)

abort unless system "git clone https://github.com/#{repouser}/homebrew-#{repo} #{tapd}"

# Work out whether we want to look at 'repo' or 'homebrew-repo' first.
`curl -Ifso /dev/null https://github.com/#{repouser}/homebrew-#{repo}`
Copy link
Member

Choose a reason for hiding this comment

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

If you're not printing the output I recommend just using quiet_system and using the boolean return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I wasn't aware of either quiet_system or safe_system. Sorry if this is a dumb question, but is there any place where "extra" internal Homebrew methods like this are documented? If not, it would be great to put on the wiki (I'm willing to help).

if $?.success?
repo = 'homebrew-' + repo
else
`curl -Ifso /dev/null https://github.com/#{repouser}/#{repo}`
Copy link
Member

Choose a reason for hiding this comment

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

And here.

@MikeMcQuaid
Copy link
Member

Made a few suggestions for changes. Want to let at least another person (probably @Sharpie or @jacknagel) merge it this time just to be safe (more sets of eyes and all that).

@telemachus
Copy link
Contributor Author

Ok, I now see that this doesn't solve the private repo problem. Private repos will always require authentication (whether cached or explicit), so I'm not sure how best to handle that.

One thought: add a flag (-p or --private) that bypasses the curl checks and simply tries to clone the repo, period. If it errors out at that point, well you asked for it and you don't have the right credentials.

This wouldn't affect existing taps at all (they're already symlinked), but it would require people using brew tap to learn a new flag for such cases.

I'm also now considering an alternative brew tap command (brew micro-tap?) which allows the user to tap anything but which requires an explicit full URL (of whatever form) rather than trying all the special-casing that goes on here. I think such a command would complement rather than replace this one.

@ghost
Copy link

ghost commented Mar 13, 2013

I can make it with full user/repo (homebrew/homebrew-dupes) or url. But would it fly here ?

@MikeMcQuaid
Copy link
Member

@telemachus Does the private repo problem come from this patch or already exist; I'm a bit confused. If it's no worse then that's ok with me.

@telemachus
Copy link
Contributor Author

@humdedum It's easy to do if we stop special-casing homebrew- in tap names. But the last I checked, @mxcl and others didn't want to to that because it's (in a way) backwards incompatible. It wouldn't actually break existing taps, but people would need to learn a new convention (namely, type the whole name).

@telemachus
Copy link
Contributor Author

@MikeMcQuaid One bug already exists, but this adds a second one.

  1. As Jack mentioned in the earlier bug report, if you have a typo in the name of what you try to tap, then git will prompt you for credentials. That bug is unfixable (I think) because of the what we're calling the "private repo" problem. See next.
  2. As Charles mentions in this thread, my impelmentation makes things worse. I use curl to test whether a repo exists, and if it doesn't, I try without homebrew-. But private repos always return 404, so both curl checks will fail. So honestly nobody should pull this version as it stands.

Having said that, I'm not sure what the best way forward is. I've already started writing an alternative brew tap command that does less special-casing, but I don't expect it to be merged (it's an alternative). As far as the main brew tap goes, it looks like the only thing that might get pulled is a change to the regex (so that taps could contain hyphens). The private repo problem means that curl can't check for repos.

@MikeMcQuaid
Copy link
Member

With main brew tap I think the least painful solution would probably just be to keep the current format requirement unless a full URL is passed in which case we tap as-is and try and convert that naming into some sensible directory name which, I'll admit, is a fair bit more work.

@telemachus
Copy link
Contributor Author

in which case we tap as-is and try and convert that naming into some sensible directory name which, I'll admit, is a fair bit more work.

Now I'm confused. Remind me, please, what do you mean by "convert that naming into some sensible directory name"?

@MikeMcQuaid
Copy link
Member

For the URL approach we'd need to convert the URL into a directory name suitable for Library/Taps.

@ghost
Copy link

ghost commented Mar 13, 2013

so just basename('.git') equivalent?

@telemachus
Copy link
Contributor Author

For the URL approach we'd need to convert the URL into a directory name suitable for Library/Taps.

Ah. I guess I would have just defaulted to treating it like basename(URL). So if it's {git,http}://long/path/to/whatever-the-hell, that would just be whatever-the-hell.

@MikeMcQuaid
Copy link
Member

I think we'd want something that would allow take the who URL into account probably so long-path-to-whatever-the-hell or even git-long-path-to-whatever-the-hell (given users don't see the full tap name very often).

@telemachus
Copy link
Contributor Author

Ok. I'm inclined to close this pull-request and rethink it all.

But before I do, can I ask interested parties to weigh in on two questions:

  1. What do people want in brew tap that it can't do now?
  2. What changes are likely to be pulled? Or, maybe more importantly, what things must not be changed?

@telemachus
Copy link
Contributor Author

I had a burst of energy, and I have a working version of an external command that extends (so to speak) brew tap.

If anyone is interested, I could use testers: https://github.com/telemachus/homebrew-anytap.

For the moment, I'm going to close this, since I can't really see any way forward at the moment, but maybe at some later time, we can try to get some of the better tested ideas from brew any-tap merged back into master.

@telemachus telemachus closed this Mar 14, 2013
Sharpie pushed a commit to Sharpie/homebrew that referenced this pull request Apr 7, 2013
Currently `brew tap` only works on repos with 'homebrew-' in their name.

This version tries the repo name as is and then falls back to try
'homebrew-repo' only if that fails.

I've also tweaked the regex  in tap_args to allow '-' in repo names. The
previous regex required a match on \w. This made it impossible for people
to tap repos with names like 'username/why-not'.

Closes Homebrew#18366.

Signed-off-by: Mike McQuaid <mike@mikemcquaid.com>
Sharpie pushed a commit to Sharpie/homebrew that referenced this pull request Apr 7, 2013
@Homebrew Homebrew locked and limited conversation to collaborators Feb 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants