Hacking on gist-logs. How do we get there?

Just through exploring the codebase a little, I can see that homebrew behaves a little differently if you have the environment variable HOMEBREW_DEVELOPER set. So that’s a neat little win.

I wanted to understand how brew dispatches all the way to gist-logs, so I could be sure I’m implementing a feature in the right place. I started out by just calling gist-logs with no arguments and received a handy error message:

usage: brew gist-logs [--new-issue|-n] <formula>

I searched for the string “–new-issue” in the brew codebase and found it’s only in the gist-logs.rb file. So that’s useful! I then got sidetracked trying to figure out why these two behaved differently:

[james@greentreeredsky brew (issue44706)]$ brew gist-logs
usage: brew gist-logs [--new-issue|-n] <formula>
b[james@greentreeredsky brew (issue44706)]$ brew gist-logs --help
Warning: No help text in: /usr/local/Library/Homebrew/cmd/gist-logs.rb
Example usage:
  brew search [TEXT|/REGEX/]
  brew (info|home|options) [FORMULA...]
  brew install FORMULA...
  brew update
  brew upgrade [FORMULA...]
  brew uninstall FORMULA...
  brew list [FORMULA...]

Troubleshooting:
  brew config
  brew doctor
  brew install -vd FORMULA

Brewing:
  brew create [URL [--no-fetch]]
  brew edit [FORMULA...]
  https://github.com/Homebrew/brew/blob/master/share/doc/homebrew/Formula-Cookbook.md

Further help:
  man brew
  brew help [COMMAND]
  brew home

It turns out homebrew has an internal help generation system! If you are writing a new tool, and you use a magic comment beginning with #:, then homebrew will automatically find the help.

Which means that the failure of the –help command is a defect! I’m an hour in and finding something to fix! Yay! I’m going to let myself get sidetracked by this, as it seems like an easy fix.

First, I added the following magic comment to the top of the file:

#: usage: brew gist-logs [--new-issue|-n] <formula>

Now both –help and no arguments behave the same, but at the cost of introducing duplication. I don’t want to introduce duplication, so I want to figure out how a different command handles both the case that someone enters the –help argument and that ARGV is empty.

Fortunately, I’m half way there. –help is handled by the magic comment and a little support in brew.rb. But the way that the empty ARGV is handled internally to gist-logs is with this little piece of logic:

 if ARGV.resolved_formulae.length != 1
  puts "usage: brew gist-logs [--new-issue|-n] <formula>"
  Homebrew.failed = true
  return
 end

So, I need to figure out how to remove that logic, and invoke the help system when there’s no arguments.

I checked the install.rb command to see how it handles this situation. It raises a FormulaUnspecifiedError exception, which is caught in brew.rb (FormulaUnspecifiedError inherits from UsageError). This dispatches to Homebrew#help, which displays the error message and the text from the FormulaUnspecifiedError exception and exits with 1. So changing the code to this seems reasonable:

raise FormulaUnspecifiedError if ARGV.resolved_formulae.length != 1

The problem here’s simple enough, though. The old implementation set Homebrew.failed = 1, but the new code does not set that. The question is, does that change matter?

From what I can tell, Homebrew.failed is used in brew.rb to simply do an “exit 1”. The help implementation also does an “exit 1”, so the command line contract is maintained, and because Homebrew.failed isn’t actually used anywhere in the new call chain, and it wasn’t _doing_ anything in the old call chain, I think the refactor is a win.

I’ll create a branch for this change and submit it to the dev team and see what they think.

And there’s the pull request: https://github.com/Homebrew/brew/pull/217

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s