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

Feature auto-stash #531

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

LukasFritzeDev
Copy link
Collaborator

The auto-stash feature calls git stash before checking for a dirty repo. It also stashes untracked/new files so the repo is really clean. After doing the other stuff the stash is applied (popped) again,
so the state of the working tree is restored.

If there are no files to stash, git stash succeds without adding a stash entry.
Therefore we have to check if the last stash is really made from Git-ftp before poping.

In addition to the option the config is supported, too.

Auto-stash is supported for actions init, push and catchup.

resolves #236

git-ftp Outdated
}

unset_stash(){
[ $AUTO_STASH -eq 1 ] && [ $(git stash list -n 1 | grep -q "Stashed by Git-ftp") ] && git stash pop -q
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will work in most cases but will go wrong from time to time (leading to weird issue reports). Example:

  • Working a feature A using auto stash.
  • Something goes wrong (network failure) and Git-ftp exits with an error.
  • I start debugging / reading docs and give up after a while.
  • I start feature B.
  • After pushing feature B, my stack from feature A is restored, possibly with conflicts.

A variation of this: I had multiple failures and several Git-ftp stash commits are piled up. They re-surface whenever I have a clean working directory.

Ideally, the stash command would tell us the commit it saved. But it doesn't. We can check the output for No local changes to save and skip the pop in that case. Or we extend the stash message with a random token and grep for that. For example:

git stash push ... -m "Stashed by Git-ftp. Token: $token"
# ...
[ $(git stash list -n 1 | grep -q "Stashed by Git-ftp. Token: $token") ] && git stash pop

But that's probably overcomplicating it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your first two cases could be handled by just calling unset_stash during cleanup which is called before the program exits with an error.

I don’t think it’s that easy to check the output string. Some people have localised versions of git so the output could be in another language and wouldn’t match. But I like the idea with the token. We could just use the current system time (mills) for this. I’ll try this.

git-ftp Outdated
set_stash() {
local stash_config="$(get_config auto-stash)"
[ -n "$stash_config" ] && AUTO_STASH="$(boolean $stash_config)"
[ $AUTO_STASH -eq 1 ] && git stash push --include-untracked -q -m "Stashed by Git-ftp"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The push option is new. My Git version doesn't know it yet. Maybe we should use git stash save for compatibility. Newer versions still recognise it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you should consider to update your git version? 😜😉

No, seriously. You're right, they still recognise it, but save is deprecated since almost two years. So I think it’s better to use the future-proof command, isn’t it?

What we could do is check the git-version and say “Oh sorry, this option is not supported for your git version. Please update git or stash your changes manually.”

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of value in long term support. Don't change a running system. That's why I'm working with Debian stable. No surprises, it just works. My current version will still be supported for another three years.

Uh, I just saw that a new Debian stable was released ten days ago. So yes, I will update my Git version along with everything else soon! :-)

LukasFritzeDev and others added 4 commits July 15, 2019 16:30
The auto-stash feature calls `git stash` before checking for a dirty repo.
It also stashes untracked/new files so the repo is really clean.
After doing the other stuff the stash is applied (popped) again,
so the state of the working tree is restored.

If there are no files to stash, git stash succeds without adding a stash entry.
Therefore we have to check if the last stash is really made from Git-ftp before poping.

In addition to the option the config is suported, too.
Add a few tests for the new auto-stash feature.
As well a test for dirty repositories.
@LukasFritzeDev
Copy link
Collaborator Author

I intentionally did the version check in an extra commit to keep it as a separate step. But If you want to, i can combine this with the token commit, because some of the changes made there are overwritten with the version check.

Since `git stash push` was introduced with v2.13 we have to check for older git versions.
This check is added and to avoid parsing the output of `git --version` multiple times, the version check is refactored.
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good. I accidentally overlooked some of the commits. Thank you for keeping on top of it.

if [ $AUTO_STASH -eq 1 ]; then
TMP_STASH_TOKEN="$(date +%s)"
local stash_message="Stashed by Git-ftp. Token: $TMP_STASH_TOKEN"
if [ "${GIT_VERSION[0]}" -le 2 ] && [ "${GIT_VERSION[1]}" -lt 13 ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, we are relying on Git not to publishing version 1.13. I guess that's a save bet. ;-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the version check in check_git_version freaked me out. I thought it would allow 1.8 first, but it doesn’t. So I decided to go a similar way here to keep it simple.

@camlafit
Copy link
Contributor

Hello

Any progress about this feature ? Could be nice to be to merge it :)
I can do some test if required

Thanks

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.

option to push latest commit
3 participants