Add HOMEBREW_UPGRADE_GREEDY="auto-updates" and "latest"

[jck112]

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

---

Fixes: #16735

[jck112]

This should also be supported in brew outdated, which also has these options.

Added in latest update.

[MikeMcQuaid]

@reitermarkus If you disagree: let's discuss here instead.

[reitermarkus]

As I mentioned in the issue, I prefer one variable since it avoids the case where multiple can be specified. For the user it's a choice between 1, latest and auto-updates.

[MikeMcQuaid]

@reitermarkus Feels a bit weird to have 1 be one of these options and also there's few variables that take multiple values like that so may be more confusing than not.

[jck112]

FWIW, the existing code does handle when multiple options are specified (e.g. brew upgrade --greedy --greedy-auto-updates --greedy-latest).

https://github.com/Homebrew/brew/blob/7848bd32269d5fcfecc00aeb142db426b9896ed6/Library/Homebrew/cask/upgrade.rb#L79-L90

If separate env vars are preferred I have a patch ready to go for that too (https://github.com/jck112/Homebrew-brew/commit/ea119ed07f3d943d0bb3fdf2f0f7b0c150f77a33).

[MikeMcQuaid]

Thanks @jck112! Will let you know when we've decided.

---

@Homebrew/maintainers @Homebrew/cask any thoughts on one variable vs. many? We don't have many variables which have differing behaviours due to setting the same variable to different values so feels unintuitive but I'm open to other viewpoints.

[miccal]

Thanks @jck112! Will let you know when we've decided.

@Homebrew/maintainers @Homebrew/cask any thoughts on one variable vs. many? We don't have many variables which have differing behaviours due to setting the same variable to different values so feels unintuitive but I'm open to other viewpoints.

I would prefer just one variable :)

[Bo98]

My general opinion (to anything):

• If the behaviour of multiple at the same time is well defined and supported (and not just a case of one overrides the other), then multiple variables makes sense
• Otherwise just a single variable is best

[reitermarkus]

Feels a bit weird to have 1 be one of these options

We can call it true or always if that makes it less weird, but the code currently accepts any value anyways.

We don't have many variables which have differing behaviours due to setting the same variable to different values

That's because we also don't have that many flags that have the same prefix with different behaviours like --greedy, --greedy-latest, --greedy-auto-updates.

The one I could find is --linux, --linux-self-hosted and --linux-wheezy in dispatch-build-bottle, but that doesn't really need a global setting.

[reitermarkus]

If the behaviour of multiple at the same time is well defined and supported

That's actually a good question. I don't think it is well defined and more of an implementation detail. --greedy implies --greedy-latest and --greedy-auto-updates, but is --greedy-latest + --greedy-auto-updates equivalent to just --greedy?

[bevanjkay]

Is --greedy-latest + --greedy-auto-updates equal to just --greedy?

Yeah, it is.

---

I think following the flags and having three variables makes sense to me.

[reitermarkus]

Yeah, it is.

Okay, if it wasn't, separate variables would make sense to me.

Given that it is equivalent, one variable makes more sense to me.

[reitermarkus]

I think following the flags and having three variables makes sense to me.

Personally, I think it would actually make more sense for the flags to be --greedy, --greedy latest and --greedy auto-updates so only one option can be specified. For now, they should at least have conflicts with each other.

[razvanazamfirei]

I recall we passed on this in #15164, but I'm glad this is discussed. Regardless, I agree with the 3 variable approach if we are keeping the flags as they are.

I think it would actually make more sense for the flags to be --greedy, --greedy latest and --greedy auto-updates

I like this approach a lot more, with the one caveat that --greedy itself is confusing and not descriptive enough to differentiate it from the other options. Perhaps having something along the lines of --greedy all as an alias for --greedy?

[jck112]

Based on my understanding of the discussion it seems the generally preferred approach would be:

1. Change --greedy to accept 3 options: all, latest, auto-updates.
2. Make -g and --greedy an alias for --greedy all.
3. Deprecate --greedy-latest and --greedy-auto-updates and make them an alias for --greedy latest and --greedy auto-updates respectively.
4. Modify HOMEBREW_UPGRADE_GREEDY to accept the same values as --greedy (i.e. "all", "latest", "auto-updates").
5. Add some error handling / messaging for invalid values passed to --greedy and HOMEBREW_UPGRADE_GREEDY.

[MikeMcQuaid]

@jck112 Sounds right to me! Don't need to be all in this PR if you don't want but, if they are, that might be nice!

[jck112]

I spent some time working on this but ran into a few implementation details that require some input:

1. Which format is preferred: "--greedy=all" or "--greedy all"? The former format is supported by an OptionalArgument type and both formats are supported by the PlacedArgument type. The type is determined internally by OptionParser by passing the format "--arg=[ARG]" or "--arg [ARG]" respectively. The parser.rb code requires moderate modifications to support PlacedArgument options.
2. A side-effect of changing --greedy to a flag, is that the short version -g can also accept an argument (e.g. -gall, -gauto-updates, -glatest). The only way to avoid this with OptionParser is to declare -g as a separate switch, which would also mean it would be listed seperately in documentation (e.g. brew help upgrade).

[^1]: A PlacedArgument supports "--arg", "--arg=ARG", and "--arg ARG". An OptionalArgument only supports "--arg" and "--arg=ARG".

[MikeMcQuaid]

@jck112 Do something like flag "--greedy=" and then args.greedy.presence&.then do |greedy| to validate the argument (like in cmd/cleanup.rb)

[jck112]

Are you okay getting rid of -g and --greedy (i.e. force users to switch to --greedy all)?

Doing flag "--greedy=" will create an option with RequiredArgument type, which means OptionParser will throw an error if the user provides --greedy without an argument:

$ brew upgrade --greedy
Error: missing argument: --greedy

[MikeMcQuaid]

Are you okay getting rid of -g and --greedy (i.e. force users to switch to --greedy all)?

@jck112 We can make it hidden: true but cannot remove it just yet (unfortunately) as we'll need to go through a deprecation cycle.

Doing flag "--greedy=" will create an option with RequiredArgument type, which means OptionParser will throw an error if the user provides --greedy without an argument:

Ah. Annoying. @homebrew/brew any thoughts on how best to address this temporarily?

[github-actions[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.