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

[Git plugin] Use stash 'push' instead of the deprecated 'save' #7486

Merged
merged 2 commits into from Apr 9, 2019
Merged

[Git plugin] Use stash 'push' instead of the deprecated 'save' #7486

merged 2 commits into from Apr 9, 2019

Conversation

brainplot
Copy link
Contributor

@brainplot brainplot commented Dec 30, 2018

CC: @ncanceill

Migrating to git stash push
As of late October 2017, there has been extensive discussion on the Git mailing list, wherein the command git stash save is being deprecated in favour of the existing alternative git stash push. The main reason for this is that git stash push introduces the option of stashing selected pathspecs, something git stash save does not support.

git stash save is not going away any time soon, so don’t worry about it suddenly disappearing. But you might want to start migrating over to the push alternative for the new functionality.

The Pro Git book, in the chapter about Stashing and Cleaning, now reports the above note. The new command was introduced with Git 2.13, which came out 2 years ago. I'm using the latest version of Git and although stash save is still present in the git-stash's man page, stash push has taken its place in the command synopsis, as a sign that the latter should be preferred.

SYNOPSIS
       git stash list [<options>]
       git stash show [<stash>]
       git stash drop [-q|--quiet] [<stash>]
       git stash ( pop | apply ) [--index] [-q|--quiet] [<stash>]
       git stash branch <branchname> [<stash>]
       git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
                    [-u|--include-untracked] [-a|--all] [-m|--message <message>]
                    [--] [<pathspec>...]]
       git stash clear
       git stash create [<message>]
       git stash store [-m|--message <message>] [-q|--quiet] <commit>

The two commands have mostly the same interface, except that with stash push the message is supplied via the -m/--message flag, while with stash save the message is the main argument of the stash command and requires no flags.

For this reason I think it's pretty safe to change the gsta alias to git stash push: the switch should be pretty seamless and hard to misuse as stash push will complain if a message is supplied with no flags as it expects a list of pathspecs.

Fixes #7474

@mcornella
Copy link
Member

I'm using 2.11.0 and it's very possible that many other people still use versions below 2.13. As I said in #7474 (comment), maybe we can just use git stash, which defaults to git stash save and git stash push.

I've also checked that flags passed to git stash are interpreted as flags passed to git stash save. It probably does the same in your version. So if you're ok with it, let's just alias it to git stash.

@brainplot
Copy link
Contributor Author

brainplot commented Jan 20, 2019

Yeah, sounds good! I'm going to force-push with the change in a minute.

EDIT: @mcornella unfortunately that's not the case for me. I did a quick test:

➜ git stash -m "Changed some stuff"
usage: git stash list [<options>]
   or: git stash show [<stash>]
   or: git stash drop [-q|--quiet] [<stash>]
   or: git stash ( pop | apply ) [--index] [-q|--quiet] [<stash>]
   or: git stash branch <branchname> [<stash>]
   or: git stash save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
                      [-u|--include-untracked] [-a|--all] [<message>]
   or: git stash [push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
                       [-u|--include-untracked] [-a|--all] [-m <message>]
                       [-- <pathspec>...]]
   or: git stash clear

The command is rejected if I pass the -m option.

@mcornella
Copy link
Member

Ok, here's what I came up with, based on git-stash from v2.11.0 and v2.13.0:

( cd -q $ZSH; git stash undef_command_to_show_usage 2>&1 | grep -q '\[push' ) && alias gsta='git stash push' || alias gsta='git stash save'

It's supper ugly, but it works. I'm not yet satisfied with the solution though. For the moment you can redefine the alias in your zshrc so you don't have to see the deprecation warning.

@mcornella mcornella added git Topic: alias Pull Request or issue regarding aliases labels Jan 20, 2019
@brainplot
Copy link
Contributor Author

I'm currently working on a solution. I'm almost there.

@brainplot
Copy link
Contributor Author

brainplot commented Jan 20, 2019

I think this is a more flexible solution. The script now parses the git version number and sets the alias accordingly.

@brainplot
Copy link
Contributor Author

I made a change so that the function no longer spawns a sed process now; everything's handled by the shell.

A utility function now parses the output of git --version and set the
alias for git stash to 'git stash push' iff the current version of Git
is greater than 2.13; it falls back to 'git stash save' otherwise.
@mcornella mcornella added the Area: plugin Issue or PR related to a plugin label Mar 24, 2019
@mcornella
Copy link
Member

I get zsh: parse error near '2.13'. If I substitute the >= with -ge (greater or equal) then it does work. I'd change from the function form to an in-line if comparison though:

[[ "$(git --version 2>/dev/null)" =~ '^git version ([0-9]+.[0-9]+)' && "$match" -ge "2.13" ]] \
  && alias gsta='git stash push' \
  || alias gsta='git stash save'

Other two options:

  • Using is-at-least:

    autoload -Uz is-at-least
    is-at-least 2.13 "$(git --version 2>/dev/null | awk '{print $3}')" \
      && alias gsta='git stash push' \
      || alias gsta='git stash save'
  • Using grep as I proposed before but with stash -h instead:

    git stash -h | grep -q '\[push' && alias gsta='git stash push' || alias gsta='git stash save'

@brainplot
Copy link
Contributor Author

I had changed it already to an inline if comparison, in my later commits.
I personally think that your first solution is better than the other two you suggested since the shell doesn't have to spawn an awk/grep process. I didn't benchmark it but I expect the first version to be quite a bit faster than the ones using awk and grep so I'd go with the first:

[[ "$(git --version 2>/dev/null)" =~ '^git version ([0-9]+.[0-9]+)' && "$match" -ge "2.13" ]] \
  && alias gsta='git stash push' \
  || alias gsta='git stash save'

Also, minor nitpicking, I don't use double-quotes where I don't need stuff to be expanded (i.e. a string literal) so I would personally write it as -ge '2.13'. It's very minor but I like to think it's faster :)

@mcornella mcornella merged commit 35dc26a into ohmyzsh:master Apr 9, 2019
@mcornella
Copy link
Member

Also, minor nitpicking, I don't use double-quotes where I don't need stuff to be expanded (i.e. a string literal) so I would personally write it as -ge '2.13'. It's very minor but I like to think it's faster :)

It's the kind of thing I would care about so don't worry there haha

I've pushed the first version then, thanks for your help!

@mcornella
Copy link
Member

Meh, I copy-pasted badly and missed a backslash. Fixed!

@brainplot
Copy link
Contributor Author

brainplot commented Apr 9, 2019

I've pushed the first version then, thanks for your help!

No problem! Thank you for merging this in!

@mcornella
Copy link
Member

It turns out it doesn't work for certain versions because it makes floating point comparisons: #7754. I'll push the is-at-least version instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: plugin Issue or PR related to a plugin Topic: alias Pull Request or issue regarding aliases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants