Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for git restore #51

Merged
merged 10 commits into from
Aug 7, 2020
Merged

Support for git restore #51

merged 10 commits into from
Aug 7, 2020

Conversation

creature
Copy link
Contributor

Git recently added a couple of new commands, to reduce overloading of git checkout: git switch (for changing branches) and git restore (for undoing changes to files). Notably, git status no longer recommends git reset HEAD foo.bar to unstage files after git adding them; instead, it recommends git restore --staged foo.bar. It'd be nice if scmpuff supported this.

This PR contains two commits:

  • 7e239a0, adding restore to the list of commands eligible to be passed to scmpuff expand --relative in git_wrapper.sh, and a test for this.
  • 6f5d2e8, which has updates to autogenerated files. bindata.go will need to be updated, for sure, as that contains a serialised copy of git_wrapper.sh.

This PR is not ready to be merged, though:

  • There's a lot of changes in bindata.go that aren't related to git_wrapper.sh, along with changes to go.mod and go.sum. I'm not sure where these have come from (I'm very new to Go) - maybe these are holdovers from an older version of the repo? Or because I'm using Go 1.14.3?
  • On my machine, the test I added does not pass. My foo.bar file remains staged.

I'd love some feedback or pointers regarding the above so I can improve it!

@mroth
Copy link
Owner

mroth commented Jun 22, 2020

Nice!

  • I suspect the unrelated changes in the bindata generated file come from using different versions of go-bindata. It's been quite a while since that file was last generated. Nothing in there looks problematic at first glance though. (Though weirdly, the new "do not edit" format line is not the typical standard...)

  • As for go.mod and go.sum changes, I think what happened was you installed go-bindata and goxc using go get while in the project directory, and Go modules system assumes you are trying to add them to the project directly so it adds them to the modules manifest? (I've gotten around this in other situations by just CD'ing to a different directory when I want to install something globally). We can/should back these changes out for now since we don't want them to be listed as base dependencies for someone to build the project.

  • Sidenote: Both go-bindata and goxc are probably getting a bit "long in the tooth" for the future, I'll probably eventually replace goxc with goreleaser like I use on other projects, and there may also be a better alternative for go-bindata these days. Not saying we should do that here now, just mentioning as a way to say "I dont think we need to worry too much about making the integration of these tools robust as part of this PR (e.g. go modules), since we might rip them out instead."

I'll take a look at the test issue soon.

@creature
Copy link
Contributor Author

Thanks for the feedback! I've backed out the changes to go.mod and go.sum. I think I've only used the rake tasks to manage the project, but my copy of the repo is old so maybe I used that previously.

@mroth
Copy link
Owner

mroth commented Jun 24, 2020

Ah gotcha.

Looking through the tests, I think it might be possible the reason this is failing on travis-ci may be it has an older version of git installed which doesnt contain the new commands? The announcement you linked seems to suggest they came out almost a full year ago though.. 🤔

@creature
Copy link
Contributor Author

creature commented Jun 25, 2020

I think it might be possible the reason this is failing on travis-ci may be it has an older version of git installed which doesnt contain the new commands?

Definitely a possibility. But the tests don't pass on my machine either – which definitely has a recent git.

Are there some intricacies to how the test environment gets its output for scmpuff init -ws? How would I check that the code under test is definitely wrapping git restore?

@mroth
Copy link
Owner

mroth commented Jun 25, 2020

Definitely a possibility. But the tests don't pass on my machine either – which definitely has a recent git.

Oops, I missed that part.

Are there some intricacies to how the test environment gets its output for scmpuff init -ws? How would I check that the code under test is definitely wrapping git restore?

Hmm. Depending on the shell, which git should return the source of the function rather than the path to git if it's loaded. But I can't think of a reason why it wouldn't be loaded in the test environment -- because if that were the case, I would think all the other tests would fail too.

If you build the binary locally, does it work if you go through the test steps manually in a shell? (versus running the tests)

This may also be a case of my old tooling scripts causing some issues in the build... I'll try to take a look at that this weekend.

@creature
Copy link
Contributor Author

If you build the binary locally, does it work if you go through the test steps manually in a shell?

It seems to work for me, yep:

[(master +) alex@MBP foo]$ export PATH="/Users/alex/go/bin:$PATH"
[(master +) alex@MBP foo]$ which scmpuff
/Users/alex/go/bin/scmpuff
[(master +) alex@MBP foo]$ eval "$(scmpuff init -ws)"
[(master +) alex@MBP foo]$ git status
On branch master
Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
	modified:   package.json

[(master +) alex@MBP foo]$ scmpuff_status
# On branch: master  |  [*] => $e*
#
➤ Changes to be committed
#
#       modified:  [1] package.json
#
[(master +) alex@MBP foo]$ scmpuff expand --relative -- restore 1
restore	package.json
[(master +) alex@MBP foo]$ git restore --staged 1
[(master *) alex@MBP foo]$ gst
# On branch: master  |  [*] => $e*
#
➤ Changes not staged for commit
#
#       modified:  [1] package.json
#

@mroth
Copy link
Owner

mroth commented Jun 26, 2020

Okay, I believe I figured it out!

The isolated test functions in a new git repo. When I did the exact test steps on a new local repo here (but without using scmpuff), I got an error on the git restore portion because there was no HEAD (since no commits have been made yet).

I made a new test that replicates the flow you showed in the output above (e..g modifying an existing committed file), which appears to pass locally:

  Scenario Outline: Wrapped `git restore` works
    Given I am in a git repository
      And a 2 byte file named "foo.bar"
      And I successfully run `git add foo.bar`
      And I successfully run `git commit -m "initial commit"`
      And a 4 byte file named "foo.bar"
      And I successfully run `git add foo.bar`

    When I run `<shell>` interactively
      And I type `eval "$(scmpuff init -ws)"`
      And I type "scmpuff_status"
      And I type "git restore --staged 1"
      And I type "exit"
    Then the exit status should be 0
    When I run `scmpuff status`
    Then the stdout from "scmpuff status" should contain:
      """
      ➤ Changes not staged for commit
      #
      #       modified:  [1] foo.bar
      """
    Examples:
      | shell |
      | bash  |
      | zsh   |

I also re-ran the go-bindata with the most recent version (installed via homebrew) which appears to have cleaned up some of the extraneous changes in the bindata file.

I can't seem to push these fixes to the branch directly, perhaps this setting needs to be enabled? https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Alternatively, I can help out if you want to make them directly.

@creature
Copy link
Contributor Author

Thanks for the troubleshooting! I've updated the test, which now passes for me locally (though not on Travis, still). I've also allowed maintainers to have access, so you can push the bindata changes to my branch. Let me know if I can help out further, here. :)

@mroth
Copy link
Owner

mroth commented Jul 8, 2020

Thanks for the troubleshooting! I've updated the test, which now passes for me locally (though not on Travis, still). I've also allowed maintainers to have access, so you can push the bindata changes to my branch. Let me know if I can help out further, here. :)

For some reason I still can't push the changes to the branch, perhaps I misunderstand the GitHub settings. Perhaps easiest if you just upgrade your copy of go-bindata (if you're on a Mac, it's in home-brew) and re-run go generate ./... which should redo the file?

For travis-ci, I believe we can opt into a more recent version of their build environment via dist: bionic (https://docs.travis-ci.com/user/reference/bionic/), which apparently should contain git 2.25 preinstalled.

@creature
Copy link
Contributor Author

For some reason I still can't push the changes to the branch, perhaps I misunderstand the GitHub settings.

Weirdly, when I came back to this pull request that flag had switched back to off. Chances are I misclicked last time – sorry for that. I've tried toggling it back on, and it seems to have stuck, so hopefully you can push to the branch. Sorry for the trouble.

Perhaps easiest if you just upgrade your copy of go-bindata?

I am on a Mac – I installed go-bindata with Homebrew. This didn't immediately work – it turns out I had an older version built in my $GOPATH. When I generated the bindata files with the newer version, most of the diff disappeared – so hopefully it's now correct. :)

Should contain a more recent version of git.
There was a previous test that only worked on recent versions of git,
so replicate that tag, but since we've now upgraded travis we can remove
the restrictions on runs there.
go-bindata wants to use the %w formatting directive for Errorf in its
generated files, which requires >= go1.13.
Go modules assumes the vendor directory is for go modules if it exists,
so lets use something else to install bundler gems as a quick workaround
This behavior appears to have changed again in more recent versions of
git, as the test case does not trigger the expected condition during
`git status --short` before even testing scmpuff. Disabling this test
for now until it can be reproduced reliably in modern git.
@mroth
Copy link
Owner

mroth commented Jul 21, 2020

@creature Phew, I believe things are working now. Take a look at the changes I pushed (mostly CI related) and let me know what you think (and that they still work for you locally). Thanks!

@creature
Copy link
Contributor Author

creature commented Aug 4, 2020

Phew, I believe things are working now. Take a look at the changes I pushed (mostly CI related) and let me know what you think (and that they still work for you locally).

Sorry for the slow reply here! Looks great to me, and the tests/updates work locally for me too.

Thanks again for the help getting this going, I'm glad to see it all squared away. :)

@mroth mroth merged commit ab65b2a into mroth:master Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants