brew.sh: don't autoupdate if --help passed

[vladshablinsky]

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew tests with your changes locally?

---

Seems to fix #1188.

[dunn]

Since it only persists for the duration of the subshell, should we just set HOMEBREW_NO_AUTO_UPDATE instead of adding another variable?

[vladshablinsky]

👍 amending.

[zmwangx]

Same goes for "-h".

[vladshablinsky]

Seems like even more than that. https://github.com/Homebrew/brew/blob/master/Library/Homebrew/brew.rb#L34

 help_flag_list = %w[-h --help --usage -?]

[zmwangx]

Yeah. -? — didn't know that. Weird one.

[reitermarkus]

Can't we set HOMEBREW_NO_AUTO_UPDATE in brew.rb by using the already existing help_flag_list instead of duplicating the flags in brew.sh, or is it already too late by then? Otherwise I would suggest removing the help_flag_list and instead exporting HOMEBREW_COMMAND_HELP like in the first diff.

[zmwangx]

By the time you get to brew.rb the battle is already lost. update-preinstall is in brew.sh.

[reitermarkus]

Ah, ok, let's get rid of help_flag_list then, as it is used only once anyways: https://github.com/Homebrew/brew/blob/master/Library/Homebrew/brew.rb#L42

[zmwangx]

Getting rid of help_flag_list and using an in place list literal is good, but I doubt we want to set another env var.

[vladshablinsky]

Getting rid of help_flag_list and using an in place list literal is good

With help_flag_list it looks more readable since variable's name explains variable's purpose.

[reitermarkus]

We could completely remove the help_flag_list by always calling the help command instead of also parsing the help flags in brew.rb.

# check for help flags and call `help` instead
arg_n=0
for arg in "$@"
do
  ((arg_n++))

  if [[ $arg = "--help" || $arg = "-h" || $arg = "--usage" || $arg = "-?" ]]
  then
    set -- "${@:1:arg_n-1}" "${@:arg_n+1}" && ((arg_n--))

    if [ $HOMEBREW_COMMAND != "help" ]
    then
      set -- "$HOMEBREW_COMMAND" "$@" && ((arg_n++))
      HOMEBREW_COMMAND="help"
    fi
  fi
done

[MikeMcQuaid]

I doubt we want to set another env var.

It's fine to set another environment variable to pass state between Bash and Ruby code. Personally I'd rather see us do that than overload HOMEBREW_NO_AUTO_UPDATE.

[vladshablinsky]

@MikeMcQuaid @reitermarkus @zmwangx @dunn

What if we transform any command like brew (--help|--usage|-?|-h|<some_command>)+ into
brew help <some_command> e.g.

brew --help install --usage -> brew help install
brew install -h -> brew help install

Basically we just remove all the help flags and unshift help to $@.

arg_n=0
for arg in "$@"
do
  ((arg_n++))

  if [[ $arg = "--help" || $arg = "-h" || $arg = "--usage" || $arg = "-?" ]]
  then
    set -- "${@:1:arg_n-1}" "${@:arg_n+1}" && ((arg_n--))

    if [[ $1 != "help" ]]
    then
      set -- "help" "$@" && ((arg_n++))
    fi
  fi
done

[MikeMcQuaid]

What if we transform any command like brew (--help|--usage|-?|-h|<some_command>)+ into
brew help <some_command> e.g.

Seems like a good idea (even if the Bash code currently makes me want to cry a bit). It may be cleaner to consider just setting HOMEBREW_HELP=1 rather than modifying the argument list.

[vladshablinsky]

It may be cleaner to consider just setting HOMEBREW_HELP=1 rather than modifying the argument list.

The idea was to prevent parsing --help-like flags twice: in sh and then in rb. Sorry for such a Bash code, I'm quite new to writing my own Bash code.

Assuming what you have said, can this be a solution?

diff --git a/Library/Homebrew/brew.sh b/Library/Homebrew/brew.sh
index 22ebb87..8ecaadd 100644
--- a/Library/Homebrew/brew.sh
+++ b/Library/Homebrew/brew.sh
@@ -179,6 +179,15 @@ then
   set -- "$@" -v
 fi

+for arg in "$@"
+do
+  if [[ $arg = "--help" || $arg = "-h" || $arg = "--usage" || $arg = "-?" ]]
+  then
+    HOMEBREW_HELP="1"
+    break
+  fi
+done
+
 HOMEBREW_ARG_COUNT="$#"
 HOMEBREW_COMMAND="$1"
 shift
@@ -268,6 +277,7 @@ setup-analytics
 report-analytics-screenview-command

 update-preinstall() {
+  [[ "$HOMEBREW_HELP" != "1" ]] || return
   [[ -z "$HOMEBREW_NO_AUTO_UPDATE" ]] || return
   [[ -z "$HOMEBREW_UPDATE_PREINSTALL" ]] || return

[MikeMcQuaid]

The idea was to prevent parsing --help-like flags twice: in sh and then in rb. Sorry for such a Bash code, I'm quite new to writing my own Bash code.

Not your bad, this stuff is just far messier to do in Bash/

Assuming what you have said, can this be a solution?

Yeh, that looks good. You could also export it and then delete the --help handling code from brew.rb in favour of checking if the environment variable is set.

[reitermarkus]

!ENV["HOMEBREW_HELP"].nil?, maybe, so we get a nice boolean.

[MikeMcQuaid]

Thanks @vladshablinsky!