RFC: scmpuff exec

[jdelStrother]

This is an attempt to sidestep limitations in the eval $(scmpuff expand -- git ... ) approach.

Instead of needing to escape the arguments and then have the shell eval them (which seems difficult/impossible in a cross-shell manner), how about having scmpuff run git internally? For example:

--- a/commands/inits/data/git_wrapper.sh
+++ b/commands/inits/data/git_wrapper.sh
@@ -11,11 +11,11 @@ if type hub > /dev/null 2>&1; then export SCMPUFF_GIT_CMD="hub"; fi
 function git() {
   case $1 in
     commit|blame|log|rebase|merge)
-      eval "$(scmpuff expand -- "$SCMPUFF_GIT_CMD" "$@")";;
+      scmpuff exec -- "$SCMPUFF_GIT_CMD" "$@";;
     checkout|diff|rm|reset)
-      eval "$(scmpuff expand --relative -- "$SCMPUFF_GIT_CMD" "$@")";;
+      scmpuff exec --relative -- "$SCMPUFF_GIT_CMD" "$@";;
     add)
-      eval "$(scmpuff expand -- "$SCMPUFF_GIT_CMD" "$@")"
+      scmpuff exec -- "$SCMPUFF_GIT_CMD" "$@"
       scmpuff_status;;
     *)
       "$SCMPUFF_GIT_CMD" "$@";;

This is just a POC - there's no tests, and exec.go is mostly a copy-paste of expand.go. If it seems like a good approach I can try to fix those.
Is scmpuff expand worth keeping? What use case does it solve that scmpuff exec can't be used for?

[mroth]

Thanks @jdelStrother for this thoughtful PoC!

I'm currently traveling and will be a little slow (I'm actually currently writing this from an airplane ✈️, please excuse any poorly formed thoughts) -- and want to think this through a bit -- it was a long while ago when I originally wrote this, and need to remember some of the reasoning.

In general, this seems like a sound approach.

The on thing I believe it would lose is the user can no longer do the full subset of any shell invocation they might normally do that would be preserved with an eval. For example, mixing scmpuff numerics with a shell history expansion such as !$ (which is last arg of previous command in bash/zsh). I can see this potentially resulting in an unexpected failure.

Another method of (possibly?) avoiding eval while preserving the above functionality may be to capture the user's $SHELL, and the have scmpuff exec call the subprocess put pass things along as an eval string to the shell, e.g. if $SHELL is bash, then something like os.Exec("bash", "-c", joinedArgs). This may (or may not) avoid the need for escaping before passing along. Of course, this then necessitates putting some awareness/handling of the different shells in the scmpuff binary itself, which may obviate some of the simplicity gains you were aiming for here.

[jdelStrother]

For example, mixing scmpuff numerics with a shell history expansion such as !$ (which is last arg of previous command in bash/zsh). I can see this potentially resulting in an unexpected failure.

I think this still works ok - with something like:

$ touch foo
$ git add !$

bash is going to expand !$ to 'foo' before calling the git function that scmpuff init -s provides. So scmpuff exec is just going to receive 'foo' as a regular argument, which doesn't need re-evaling by bash.

[jdelStrother]

I've added a few tests and refactored the shared exec/expand code to a new arguments package. Anything else I can do to help merge this?

[mroth]

Thanks @jdelStrother, this is looking good. The new package does help DRY things up. Thanks for the bump.

• In addition, I'm wondering if we can add a E2E feature test for the case of mixing shell built-ins with numeric expansion execs, e.g. the !$ option above (though maybe you can think of a more appropriate example than me here to test this), which will help us validate if this works across many different shells.

• You astutely ask the question of whether scmpuff expand still serves a purpose once exec exists. I think that's a question worth us answering, because having two similar commands that do things slightly differently will make things confusing. Can you think of any use cases where expand is still valuable as a standalone? If not, what do you think of the idea of it becoming a sub-flag of exec, e.g. scmpuff exec --expand foo, "echo expanded command to stdout rather than executing" or something like that? That could then preserves it in a semi debugging-ish usage (or supporting use cases we didn't think about), but exec becomes the default new way of doing things.

^^^ EDIT: in retrospect, after hitting enter, I'm now wondering if scmpuff expand --exec foo is more ergonomic for folks to wrap their heads around from a UX perspective than the inverse in bullet two? (Though it doesn't reflect what's happening internally with the program very well.)

[mroth]

Not too much in code review -- looks clean!

[mroth]

Since this isn't a command itself, the package should not exist in the top-level commands hierarchy. I'd put it in commands/internal/arguments (the internal directory has special meaning, see: https://blog.learngoprogramming.com/special-packages-and-directories-in-go-1d6295690a6b), which allows other command packages to import it and share the code but doesn't make it globally exported.

[mroth]

In addition, please add a docstring comment of the syntax // Package arguments ... in order to appease go-lint. A simple line or two describing the purpose of the package will suffice.

[jdelStrother]

Will do, thanks. I’m new to go so wasn’t sure of the best way of packaging it

[jdelStrother]

This is done, although I wasn't able to get go-lint to produce a warning if that comment was missing, so not totally sure I've fixed it correctly

[jdelStrother]

In addition, I'm wondering if we can add a E2E feature test for the case of mixing shell built-ins with numeric expansion execs, e.g. the !$ option above (though maybe you can think of a more appropriate example than me here to test this), which will help us validate if this works across many different shells.

I did something for this here - 9fe9e4d . I'm not totally sure about it:

• I can't persuade !$ to work in cucumber, I'm not totally sure why. Something about the way it's invoking the commands..? Instead I've set an environment variable called FILE and used that (git add "$FILE" 2)
• I feel like this is testing shell behaviour rather than anything to do with scmpuff, but happy to leave it in if you think it's useful.

what do you think of the idea of it becoming a sub-flag of exec, e.g. scmpuff exec --expand foo,

We could do... One of my reservations is that exec & expand still have subtle differences (eg, expand's handling of empty args -

scmpuff/commands/expand/expand.go

Lines 66 to 69 in 2233a2c

	if processed == "" {
	processed = "''"
	}

), which might complicate the code a little if we try and merge their code paths into a single command.

I think it also depends on how careful you want to be about backwards compatibility. If someone does have a shell script that uses scmpuff expand, presumably we want that to continue working as-is, right? That would probably rule out the scmpuff exec --expand foo option, though honestly I can't think of a single good use for expand going forward.

[jdelStrother]

If I rebase this, can we get it merged?

[mroth]

Sincere apologies for the delay! Yes, please rebase and check for conflicts, and I'll review to refresh my memory and see if I can help out with those open questions for the Cucumber tests. I should hopefully have some spare time next week to do so.

[jdelStrother]

I've rebased - let me know if there's any else I can do to help merge.

[mroth]

This is looking good to me and I think it's ready to merge! 🎉

We'll probably want to cut a release shortly thereafter. (I'm wondering if it makes sense to do a "release candidate" build of some kind to verify no unexpected behavior is hitting people with edge case setups? Though I don't know if we'd have enough RC testers to actually get that feedback in a timely fashion.)

[jdelStrother]

We'll probably want to cut a release shortly thereafter. (I'm wondering if it makes sense to do a "release candidate" build of some kind to verify no unexpected behavior is hitting people with edge case setups? Though I don't know if we'd have enough RC testers to actually get that feedback in a timely fashion.)

I'd love to get fish support into that release if possible - I have a branch for that which builds on top of this one. I'll try and get that tidied up this weekend.

[mroth]

@jdelStrother it's been a year and neither of us has been doing much on this repo -- should we potentially go ahead and re-visit doing this merge now? (I'm thinking of doing some build tooling cleanup in this repo to get it to build macOS ARM builds for this with M1 chipsets, but would want to try to get the baseline app in a stable state first.)

[jdelStrother]

Sure, yes please. FWIW since then I've been using https://github.com/jdelStrother/scmpuff/commits/master daily without running into any issues - that's just this branch with added support for --shell=fish

[mroth]

@jdelStrother Awesome. Can you please rebase this PR branch off the latest origin/master so we see the tests run on the new CI infra, and I'll merge once it passes? (I thought there was a way for me to do that as maintainer, but can't seem to find it.)

(After that, perhaps we should start a new issue to work on getting the fish support into main as well!)

[mroth]

I manually rebased this. Weirdly enough, it's passing integration tests on both macOS/Linux in CI, but 3 tests fail on my local dev environment (in a Linux docker devcontainer).. My bad, I had an older binary in the path that was interfering with the local integration tests. Yay for CI.