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?

---

When fetching git repos, automatically correct local branch name if we find the remote HEAD was renamed.

---

Completely reworked PR; to the point that my original comment doesn't make sense anymore. Original comment follows.

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.

[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)

[rrotter]

Rewrote this based upon feedback. As is, this will handle renames of third party and non-core taps.

This can handle renaming the brew repo and core repo as well, but we run into edge cases when $SKIP_FETCH_BREW_REPOSITORY or $SKIP_FETCH_CORE_REPOSITORY get set (namely, if we're using a mirror, and the more marginal case that the git repo isn't initialized).

I'd like to look at those edge cases a bit more before marking this as "Ready for review". There is probably another PR in order for fixing the handling of repo initialization wrt non-master branch names.

[rrotter]

This now handles branch renames for all taps as well as brew itself, including the edge cases I mentioned in my last post.

Additions are entirely agnostic as to what the upstream branch is called: renames to/from master/main/whatever are all handled the same way.

I found several adjacent issues with how the update code handles branch names, and this PR snowballed to be a lot larger than expected. I can break off parts of this into separate PRs if you prefer.

[MikeMcQuaid]

I found several adjacent issues with how the update code handles branch names, and this PR snowballed to be a lot larger than expected. I can break off parts of this into separate PRs if you prefer.

Yes please 🙇🏻. With PRs like this: as many small PRs as possible is ideal.