-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Description
The app currently determines the context it's operating under from several sources:
- the filesystem (e.g. current directory name)
- app config file (currently
~/.config/hubin YAML format) - environment variables (e.g.
GH_REPO,GITHUB_TOKEN, etc.) - git config (via
git config&git remote -v, sources:.git/config&~/.gitconfig) - git tree (e.g. current branch, current commit SHA)
- SSH config (sources:
~/.ssh/config&/etc/ssh_config) - resolves SSH aliases for hostnames listed under git remotes - GitHub API (e.g. info about the current repo)
When something like gh pr create gets run, quite a lot happens under the hood. For example:
- Current GitHub user and OAuth token are obtained from app config;
- The list of git remotes gets queried and parsed;
- The "main" remote (i.e. one pointing to the canonical GitHub repo) is determined by searching for the first one in this list:
upstream,github,origin; - The base branch is determined via
git symbolic-ref refs/remotes/<REMOTE>/HEAD(alternatively, by querying repo information via API); - The head branch is determined by looking at the push target for the current branch:
- explicit upstream branch configuration is first looked up;
- otherwise, the first remote that has a same-named tracking branch is the likely push target;
- otherwise, assume the branch isn't pushed yet, so determine the first remote that points to a GitHub project that the current user has write capabilities to;
- if such a remote doesn't exist, create one by forking;
- A person's preferred text editor is looked up for authoring PR description text;
- PR creation operation proceeds.
To facilitate all these lookups, the current codebase has a loose system of mapping one piece of information to another. For example:
- current git repo 👉 default ("base") branch
- current git repo 👉 "main" remote
- git remote 👉 GitHub repository (a.k.a. "project") it maps to
- tracking branch 👉 git remote it belongs to
- current user 👉 the person's fork
Since a lot of lookups start from the current git repo (based on the current working directory at the time that the CLI app runs), there is a LocalRepo struct that encapsulates performing some of these mappings, while additional mapping logic is scattered across individual methods such as Branch.RemoteName(), Remote.Project(), etc. Some problems I find with the current system:
- Inconsistent naming (e.g. "repository" vs. "project", the ambiguity of "branch")
- Blurred responsibility between objects (e.g. why would a Branch have to know how to map itself to a Remote, or a Remote to a Project?)
- Methods that do too much but don't sufficiently betray intent (e.g.
LocalRepo.RemoteBranchAndProject()) - Hard to stub out in tests (ideally, unit tests should be able to set up a mock context in memory rather than having to write a test git repo out to fileystem)
- All of this code is under the same Go package: "github".
My rough proposal for starting to address this:
- Get rid of methods and structs named "Project" from the codebase;
- Get rid of LocalRepo;
- Design an abstraction around
git configthat is able to mock responses in memory; - Consider moving
github/branch.goto under the "git" package; - Consider isolating
git/ssh_config.goto its own package; - Minimize the implementation of Remote and instead perform necessary mappings through a separate service object;
- Minimize the "github" package until it's ideally gone;
- Write Go unit tests along the way to confirm the testability of these implementations.
Let's revise all this as we go along! Thank you for reading. 🙇