automatically repair tap with renamed main branch

[rrotter]

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

---

First step to fixing #17296.

We already have robust support for both detecting and fixing taps with renamed upstream HEAD. This puts those two together.

• support repairing one tap at a time with brew tap --repair foo/bar
• in update.sh, run brew tap --repair on any taps where we detect renamed upstream HEAD
• surpress git errors on first git fetch attempt, only if we detect rename
• after brew tap --repair, retry git fetch

[MikeMcQuaid]

Looks good so far! Are there any performance implications for brew update in the non-repair case? Can test this out with hyperfine.

[rrotter]

Looks good so far! Are there any performance implications for brew update in the non-repair case? Can test this out with hyperfine.

This should cost roughly 10ms per tap, because we're running git symbolic-ref refs/remotes/origin/HEAD an extra time on each tap. I have 4 taps installed, so I should see hyperfine 'brew update' running 40ms slower, but it's hard to tell because 40ms is within the margin of error. I'm getting results around 2.1s ± 0.300s for both this branch and master.

The extra git symbolic-ref run is because we can't tell whether the cached output of this command is safe to use unless we know whether HEAD was renamed, and because the HEAD rename happens in a subshell, we don't know whether a rename occured by the time we need to read the cache out.

The use of this cache was removed in the first commit on this branch. I just removed the creation of this cache in my latest commit/push because it's an unused variable. If you're concerned about the time savings I could revert the last commit and use a temp file to track any renames; then we could invalidate the cache only when we need to. This would save some time, at the cost of complexity.

Any thoughts? I'm happy to implement this either way, but I slightly prefer it in the current state I've committed: the cache I removed provided a tiny time savings and made code harder to read and debug.

[MikeMcQuaid]

The extra git symbolic-ref run is because we can't tell whether the cached output of this command is safe to use unless we know whether HEAD was renamed, and because the HEAD rename happens in a subshell,

Feels like it might be nicer/safer to do this rename not in a subshell and set some marker file indicating that we don't need to check it again every brew update run?

Or is this the caching you're referring to above?

In my mind (correct me if wrong!) the ideal flow would be something like:

• we fetch all the taps (but don't checkout yet)
• if the expected branch name no longer exists (e.g. master) then we look at the new upstream HEAD name and use that instead
• if the expected branch name exists, the existing functionality should be more-or-less as-is

Thoughts?

Thanks for the PR!

[rrotter]

@MikeMcQuaid, that's an accurate summary of the workflow. That's what we were already doing before this PR, except we were printing an error annoucing that we discovered the rename rather than fixing it.

Latest push is a refactor to:

• move brew tap --repair out of subshell. This was a good suggestion, and while it increases the diff size, I think the resulting code is cleaner, and of course that fixes the issue with variable scope discussed above.
• also --repair taps during auto update (I think this is what we want, otherwise someone who only ever runs brew install would eventually have their taps break)
• small tweak to output of brew update --verbose

To be clear, we're not running any extra code in the base case (when taps are reachable and not renamed). The rename check was always taken care of in error handling, and nothing in this PR changes that.

[MikeMcQuaid]

Thanks @rrotter! Just to set expectations: because this is update code and any breakage here potentially requires manual intervention for all Homebrew users: I'm going to be very conservative with review. Apologies for that.

[MikeMcQuaid]

Sorry if I didn't say this clearly enough before: we can't/shouldn't call brew tap inside brew update like this. Instead, this should perform the same operations that brew tap --repair would but in raw Bash.

[MikeMcQuaid]

Is this function extraction essential to landing this PR? As-is: it's very hard to visually differentiate what changes have been made here. It might be best to extract this into the separate with zero logic changes in a separate PR that lands before this one.

[rrotter]

Just to set expectations: because this is update code and any breakage here potentially requires manual intervention for all Homebrew users: I'm going to be very conservative with review. Apologies for that.

As a homebrew user, I'd expect nothing less. Thank you for your forthrightness.

(I'll follow up on the code comments later today or over the weekend)