Refactor GitRepositoryExtension to avoid monkey-patching

[dduugg]

• 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?

---

This refactors GitRepositoryExtension as a static module, rather than one that was dynamically extended by a Pathname. This approach allows us to enable typing, remove an .rbi file, and make the code more comprehensible (it wasn't clear which Pathname objects had this add'l behavior).

Note that Library/Homebrew/extend/git_repository.rb and Library/Homebrew/utils/git_repository.rb have considerable overlap, but i haven't attempted to unify them in this PR. (The extend version lacks test, the utils version is part of the public API, and they diverge in subtle ways. For instance, both have a git_commit_message but with opposite defaults for the safe kwarg.)

[BrewTestBot]

Review period will end on 2023-03-23 at 04:08:15 UTC.

[MikeMcQuaid]

Nice work @dduugg! Just thinking if you're going to refactor all these call-sites: I think some of the naming/API could be tweaked at the same time to make it a little nicer to use.

[dduugg]

PTAL @MikeMcQuaid @RandomDSdevel
(I haven't undertaken any method renaming, since we aren't stamping Git in front of every command anymore with this change in implementation)

[RandomDSdevel]

     Don't mind me, I just had the same thought as the one @MikeMcQuaid did in that review comment of his that I gave a thumbs-up reaction to. Some other thoughts:

• Consider splitting any name changes out of this PR, maybe? 'Git repository' is a sort of domain term of art, so it might be slightly confusing to refer to one as a 'Git path.' 'Git repository's path' or 'path to a/the Git repository' works. I don't, by any means, necessarily really mean for this to be taken as a blocking comment to any extent, though.
• Independently of that, are there any formulas or casks that use this code, by any remote chance? It could well bw surprising if they did, but that be something to double-check, perhaps?

[dduugg]

Solid thought, GitPath could be confused for the path to the git executable itself, so GitRepoPath could be less ambiguous choice. The module is marked @api private, so I assume it's fair game, but I don't know what kind of escape hatches might exist (Hyrum's Law and all).

[carlocab]

Suggested change

	class GitPath < SimpleDelegator
	class GitRepository < SimpleDelegator

Maybe? GitRepoPath is fine too.

[BrewTestBot]

Review period ended.

[MikeMcQuaid]

Consider splitting any name changes out of this PR, maybe?

I disagree. Makes sense to do all this refactoring at once.

Independently of that, are there any formulas or casks that use this code, by any remote chance?

Not in homebrew/core. It's not marked as a public API so if it goes 💥 beyond that: that's not on us.

[MikeMcQuaid]

Suggested change

	def git_repo?
	def exist?

or something feels like it makes more sense with this naming. GitRepoPath.new(repo).git_repo? feels pretty repetitive?

[dduugg]

GitRepoPath.new(repo).git_repo? is repetitive, yes, but the problem is in Library/Homebrew/tap.rb. I think some minor redundancy elsewhere is preferable to the below, wdyt?

diff --git a/Library/Homebrew/tap.rb b/Library/Homebrew/tap.rb
index d1e9c4850..4c17062f6 100644
--- a/Library/Homebrew/tap.rb
+++ b/Library/Homebrew/tap.rb
@@ -135,7 +135,7 @@ class Tap
   def remote
     return default_remote unless installed?

-    @remote ||= path.git_origin
+    @remote ||= path.origin
   end

   # The remote repository name of this {Tap}.
@@ -163,28 +163,28 @@ class Tap

   # True if this {Tap} is a Git repository.
   def git?
-    path.git_repo?
+    path.repo?
   end

   # git branch for this {Tap}.
   def git_branch
     raise TapUnavailableError, name unless installed?

-    path.git_branch
+    path.branch
   end

   # git HEAD for this {Tap}.
   def git_head
     raise TapUnavailableError, name unless installed?

-    @git_head ||= path.git_head
+    @git_head ||= path.head
   end

   # Time since last git commit for this {Tap}.
   def git_last_commit
     raise TapUnavailableError, name unless installed?

-    path.git_last_commit
+    path.last_commit
   end

   # The issues URL of this {Tap}.
@@ -385,20 +385,20 @@ class Tap
       $stderr.ohai "#{name}: changed remote from #{remote} to #{requested_remote}" unless quiet
     end

-    current_upstream_head = path.git_origin_branch
-    return if requested_remote.blank? && path.git_origin_has_branch?(current_upstream_head)
+    current_upstream_head = path.origin_branch
+    return if requested_remote.blank? && path.origin_has_branch?(current_upstream_head)

     args = %w[fetch]
     args << "--quiet" if quiet
     args << "origin"
     safe_system "git", "-C", path, *args
-    path.git_origin_set_head_auto
+    path.origin_set_head_auto

-    new_upstream_head = path.git_origin_branch
+    new_upstream_head = path.origin_branch
     return if new_upstream_head == current_upstream_head

-    path.git_rename_branch old: current_upstream_head, new: new_upstream_head
-    path.git_branch_set_upstream local: new_upstream_head, origin: new_upstream_head
+    path.rename_branch old: current_upstream_head, new: new_upstream_head
+    path.branch_set_upstream local: new_upstream_head, origin: new_upstream_head

     return if quiet

[MikeMcQuaid]

I dunno, I kinda like the abbreviated version. If we wanted to improve it: I wonder if aliasing path to git_path or similar would be nicer?

[MikeMcQuaid]

Re-reading the above: yeh, I agree moreso with this that it's confusing that path doesn't return a Pathname. Feels like this method should have git_ somewhere in it as it's now returning a GitRepoPath

[MikeMcQuaid]

I'm sorry this is taking so much back and forth!

This now looks pretty weird?

I'd expect that source_tap.path returns a Pathname. If not, maybe something like

Suggested change

	repo = source_tap.path.pathname
	repo = source_tap.git_repo.path

or

Suggested change

	repo = source_tap.path.pathname
	repo = source_tap.git_repo_path.pathname

would make more sense?

[dduugg]

Yeah, we can do that. This is just being explicit for the type checker though, since .path delegates methods to .pathname anyay. We could equivalently update relevant sigs to allow GitRepoPath along with Pathname, or use T.cast to overrule sorbet where necessary. (I'm happy to do the suggest refactor, just providing some context.)

[MikeMcQuaid]

I'm fine with either my suggestion or the sig one? I think T.cast is better avoided when we can (it's a bit ugly).

[MikeMcQuaid]

Re-reading the above: yeh, I agree moreso with this that it's confusing that path doesn't return a Pathname. Feels like this method should have git_ somewhere in it as it's now returning a GitRepoPath

[dduugg]

Going to park this one for a bit in favor of closing out some easier-to-type files

[MikeMcQuaid]

@dduugg Let's leave this open if that's ok! I can tone down the bike-shedding on the naming 😅, just would be good to land this in some form given how much review there has been.

[dduugg]

I've renamed path: to git_repo: and removed the default value (it was never valid bc it was a String). Same for squash_package_commits below.

[dduugg]

PTAL @MikeMcQuaid
I finally had the chance to come back to this, and I believe that I've addressed all of the outstanding feedback. Let me know what you think.
GitRepository is now a standalone class without delegation. To do this, I added a new ivar to Tap, and updated all the call sites depending on whether they were using git or ordinary Pathname methods. I've updated the method names as well, based on earlier suggestions.

[MikeMcQuaid]

Looks great, thanks for this @dduugg!