feat: add support for git branch #98
Conversation
hansonw
changed the title
|
Very cool! Thanks for such a detailed PR. It might take me a little while to review this, hopefully @hansonw you might be up for a few rounds of back and forth so we can figure out how this would interface with the broader refactor of the codebase? I really appreciate this! |
| } | ||
|
|
||
| func gitBranchOutput() []byte { | ||
| out, err := exec.Command("git", "branch", "--color=never").Output() |
There was a problem hiding this comment.
It looks like there unfortunately isn't a --porcelain (guaranteed not to change across git versions) version of git branch output like there is with git status? I'm wondering how to make the parsing resilient to multiple versions of git in the wild (including future versions). Of course, once I get around to #33 that could be a good long term solution. Another possibility while relying on CLI git, do you think it might make sense to use something like --format to specify a specific format? I'm not sure if that might be more resilient to change.
There was a problem hiding this comment.
Another edge case to consider might be whether there is a way to ensure the pager is disabled on a per command basis here, so git doesn't try to run its output through a pager if there more lines of branches than it thinks will fit on the user screen. From the git branch documentation it appears that is the default, we might want to verify if it is automatically disabled somehow based on detecting whether STDOUT is a pipe or tty.
There was a problem hiding this comment.
--format w/ pager forcibly disabled makes sense to me here!
| let e++ | ||
| (( e++ )) |
There was a problem hiding this comment.
Is this syntax fully POSIX compatible?
There was a problem hiding this comment.
This was automatically flagged by the shell linter you have in the repo, can’t see the exact message any more but I believe it mentioned that this has been supported in Bash for decades
| @@ -22,6 +22,28 @@ function scmpuff_status | |||
| end | |||
| end | |||
|
|
|||
| function scmpuff_branch | |||
| scmpuff_clear_vars | |||
| set -lx scmpuff_env_char "e" | |||
There was a problem hiding this comment.
Clobbering the existing file variable could be problematic: it might not be obvious to a user that if they list branches, the previous file shortcuts that were displayed to them would stop. working. I suspect we need a second variable here (which unfortunately does add to implementation complexity, but I don't know a great way around that).
There was a problem hiding this comment.
I was previously a very active user of scm_breeze (I definitely prefer the simplicity of scmpuff!) and fwiw it shared the same env var space between all numbered commands. It definitely wasn’t ideal but was pretty intuitive I think
| @@ -0,0 +1,45 @@ | |||
| Feature: branch command | |||
There was a problem hiding this comment.
Really appreciate these integration tests!
|
Thanks for reviewing! If you’re in the middle of a big refactor maybe it’s easier to just wait until after that lands to make this change? full disclosure this PR was 95% generated by OpenAI Codex (which I work on ;)) so nbd to just do it again later! |
Thanks for this tool! this implements #20 :)
Summary
branchsub-command that prints git branches with numbersgbas an alias forscmpuff_branchin the initialization scriptsTesting
Example