bottle: prioritize HOMEBREW_CELLAR over :any over :any_skip_relocation

[dawidd6]

• 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 style with your changes locally?
• Have you successfully run brew tests with your changes locally?

---

This is an example from @sjackman on what is the issue:

❯❯❯ jq '."brewsci/bio/ntcard".bottle.cellar' ntcard--1.2.0.catalina.bottle.json ntcard--1.2.0.x86_64_linux.bottle.json
"any"
"any_skip_relocation"
❯❯❯ brew bottle --merge --write *.json
==> brewsci/bio/ntcard
  bottle do
    root_url "https://linuxbrew.bintray.com/bottles-bio"
    cellar :any_skip_relocation
    sha256 "3687ad1e69cfc8849b4d9d6c2aaaf96f426125509658ace1ec4febe7c3dadcf3" => :catalina
    sha256 "a4c224a29010985c7956a6ef37e6b08f3370975512031f16ed5f1370b55866a3" => :x86_64_linux
  end

As one can see, there are two different cellars and the less proper one is chosen.
After applying the patch from this PR:

➜  ~/Downloads  brew bottle --merge ntcard--1.2.0.catalina.bottle.json ntcard--1.2.0.x86_64_linux.bottle.json 
==> brewsci/bio/ntcard
  bottle do
    root_url "https://linuxbrew.bintray.com/bottles-bio"
    cellar :any
    sha256 "3687ad1e69cfc8849b4d9d6c2aaaf96f426125509658ace1ec4febe7c3dadcf3" => :catalina
    sha256 "a4c224a29010985c7956a6ef37e6b08f3370975512031f16ed5f1370b55866a3" => :x86_64_linux
  end
➜  ~/Downloads  brew bottle --merge ntcard--1.2.0.x86_64_linux.bottle.json ntcard--1.2.0.catalina.bottle.json 
==> brewsci/bio/ntcard
  bottle do
    root_url "https://linuxbrew.bintray.com/bottles-bio"
    cellar :any
    sha256 "3687ad1e69cfc8849b4d9d6c2aaaf96f426125509658ace1ec4febe7c3dadcf3" => :catalina
    sha256 "a4c224a29010985c7956a6ef37e6b08f3370975512031f16ed5f1370b55866a3" => :x86_64_linux
  end

[sjackman]

Thanks, Dawidd!

[MikeMcQuaid]

Not sure I understand the .min here. Can you explain?

[dawidd6]

It will return the minimum value from those 2 cellars.
HOMEBREW_CELLAR < :any < :any_skip_relocation.

That was @sjackman idea and it works well.

[MikeMcQuaid]

Is that just based on a string comparison? If so it's a bit confusing and I think an if/else or a case statement might be a bit more obvious with intent.

[sjackman]

Yep, just based on string comparison. /… sorts before any sorts before any_skip_relocation. I'm personally okay with this style, so long as the comment precedes the code. The code to manually sort it could be a bit tedious. If we go that route, I'd suggesting creating a comparison function for this member and call [first, second].min { |a, b| … }

[sjackman]

@MikeMcQuaid Your call. What do you think?

[dawidd6]

Please do.

[MikeMcQuaid]

If I've understood correctly: how about:

Suggested change

	[first, second].min
	cellars = [first, second]
	if cellars.include?(HOMEBREW_CELLAR)
	HOMEBREW_CELLAR
	elsif cellars.include?(:any)
	:any
	elsif cellars.include?(:any_skip_relocation)
	:any_skip_relocation
	else
	second
	end

[sjackman]

This code doesn't handle the case when the cellar is the default path from the other OS or a non default path. How about:

if cellars.include?(HOMEBREW_CELLAR)
  HOMEBREW_CELLAR
elsif first.start_with?("/")
  first
elsif second.start_with?("/")
  second
elsif cellars.include?(:any)
  :any
else
  :any_skip_relocation
end

[MikeMcQuaid]

Would probably want to see an elsif cellars.include?(:any_skip_relocation) partly just to ensure this code survives being changed in future (as an else it makes me a bit uneasy).

[dawidd6]

Updated.

[MikeMcQuaid]

Nice work, thanks @dawidd6!

[sjackman]

Thank you, Dawid!