Re: [PATCH] stash: allow ref of a stash by index
Øystein Walle writes: > diff --git a/git-stash.sh b/git-stash.sh > index 826af18..b026288 100755 > --- a/git-stash.sh > +++ b/git-stash.sh > @@ -384,7 +384,7 @@ parse_flags_and_rev() > i_tree= > u_tree= > > -REV=$(git rev-parse --no-flags --symbolic --sq "$@") || exit 1 > +REV=$(git rev-parse --no-flags --symbolic --sq "$@" 2>/dev/null) > > FLAGS= > for opt > @@ -422,6 +422,15 @@ parse_flags_and_rev() > ;; > esac > > +case "$1" in > +*[!0-9]*) > +: OK, so you ignore anything that has a non-digit here, to ensure that... > +;; > +*) > +set -- "${ref_stash}@{$1}" ... this one triggers only for a string $1 that consists solely of digits. > +;; > +esac Makes sense. I notice that both of the two existing case/esac statements in this function indent case arms and their bodies one level too deep, which you follwed with the above addition. That may be something we would want to fix in a follow-up patch. > REV=$(git rev-parse --symbolic --verify --quiet "$1") || { > reference="$1" > die "$(eval_gettext "\$reference is not a valid reference")" I agree with Peff that the change in error message he noticed is probably an improvement ;-) Want to do a final log message to make it a real patch? Thanks.
Re: [PATCH] stash: allow ref of a stash by index
On Mon, Sep 05, 2016 at 11:46:34PM +0200, Øystein Walle wrote: > The bash-specific code is a no-go, so here's a way to do it in a way > that I think is in line with Git's code style for shell scripts. I took > the liberty of removing the '|| exit 1' since the rev is verified later > on anyway, as can be seen in the last piece of context. That way the > argument munging can be done at a later stage where we don't have to > loop over multiple ones. The first rev-parse's purpose is just to apply > --sq. I wondered how that would impact the error message when there is no such stash. It looks like rev-parse will still return the bogus name on stdout, so we do not run afoul of the "No stash found" code path. We do get: $ git.compile stash show foobar foobar is not a valid reference instead of: $ git stash show foobar fatal: ambiguous argument 'foobar': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git [...] -- [...]' But I do not see that as a big downside (I might even call it an improvement). -Peff
Re: [PATCH] stash: allow ref of a stash by index
Pasting text into Gmail's web interface is not conducive to sending tabs through the intertubes. But you get the idea.. Øsse
Re: [PATCH] stash: allow ref of a stash by index
Hi, guys I like this idea. It makes stash easier and quicker to use, and it "hides" the fact that it uses the reflog for keeping track of the made stashes. *Not* to say I discourage interested people from peeking under the hood. I just think it's nice to sometimes think of the stash as a separate concept instead of being built on top of strange merge commits constructed in temporary indexes :) The bash-specific code is a no-go, so here's a way to do it in a way that I think is in line with Git's code style for shell scripts. I took the liberty of removing the '|| exit 1' since the rev is verified later on anyway, as can be seen in the last piece of context. That way the argument munging can be done at a later stage where we don't have to loop over multiple ones. The first rev-parse's purpose is just to apply --sq. (Besides, the only way to do it at the top like in the original patch was for arg in bleh "$@" where bleh is a marker to indicate that the positional arguments should be cleared in a separate case branch so that they can be rebuilt with multiple invocations of set later. Not pretty, in my opinion.) Regards, Øsse diff --git a/git-stash.sh b/git-stash.sh index 826af18..b026288 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -384,7 +384,7 @@ parse_flags_and_rev() i_tree= u_tree= -REV=$(git rev-parse --no-flags --symbolic --sq "$@") || exit 1 +REV=$(git rev-parse --no-flags --symbolic --sq "$@" 2>/dev/null) FLAGS= for opt @@ -422,6 +422,15 @@ parse_flags_and_rev() ;; esac +case "$1" in +*[!0-9]*) +: +;; +*) +set -- "${ref_stash}@{$1}" +;; +esac + REV=$(git rev-parse --symbolic --verify --quiet "$1") || { reference="$1" die "$(eval_gettext "\$reference is not a valid reference")"
Re: [PATCH] stash: allow ref of a stash by index
From: "Johannes Schindelin" Hi, On Sat, 3 Sep 2016, Jeff King wrote: On Sat, Sep 03, 2016 at 07:21:18PM -0400, Aaron M Watson wrote: > Allows stashes to be referenced by index only. Instead of referencing > "stash@{n}" explicitly, it can simply be referenced as "n". This says "what" but not "why". I assume it is "because the former is more annoying to type". Are there any backwards-compatibility issues you can think of? I think that "123456" could be a sha1, but I do not see much point in referencing a sha1 as the argument of "stash show". And it looks like this code path is called only from is_stash_like(), so presumably the same logic would apply to other callers. Maybe we could make it unambiguous, e.g. by using # instead: #123456 cannot refer to a SHA-1. The alternative is to limit the length to less that the shortest ambiguous sha1 length that has been used (by Git users - 5, 6, 7? )? Which is probably allowing 1-4 characters, which is a reasonbly deep stash index... If you need to refer to stash@{9362} you have bigger problems. But then, '#' are comment-starting in shells, so they would have to by escaped. Maybe the best option would be to introduce a -n option, with the shortcut - thanks to e0319ff (parseopt: add OPT_NUMBER_CALLBACK, 2009-05-07). Ciao, Johannes
Re: [PATCH] stash: allow ref of a stash by index
Hi, On Sat, 3 Sep 2016, Jeff King wrote: > On Sat, Sep 03, 2016 at 07:21:18PM -0400, Aaron M Watson wrote: > > > Allows stashes to be referenced by index only. Instead of referencing > > "stash@{n}" explicitly, it can simply be referenced as "n". > > This says "what" but not "why". I assume it is "because the former is > more annoying to type". > > Are there any backwards-compatibility issues you can think of? > > I think that "123456" could be a sha1, but I do not see much point in > referencing a sha1 as the argument of "stash show". And it looks like > this code path is called only from is_stash_like(), so presumably the > same logic would apply to other callers. Maybe we could make it unambiguous, e.g. by using # instead: #123456 cannot refer to a SHA-1. But then, '#' are comment-starting in shells, so they would have to by escaped. Maybe the best option would be to introduce a -n option, with the shortcut - thanks to e0319ff (parseopt: add OPT_NUMBER_CALLBACK, 2009-05-07). Ciao, Johannes
Re: [PATCH] stash: allow ref of a stash by index
Jeff King writes: >> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt >> index 92df596..af11cff 100644 >> --- a/Documentation/git-stash.txt >> +++ b/Documentation/git-stash.txt >> @@ -35,11 +35,12 @@ A stash is by default listed as "WIP on >> 'branchname' ...", but >> you can give a more descriptive message on the command line when >> you create one. >> >> -The latest stash you created is stored in `refs/stash`; older >> -stashes are found in the reflog of this reference and can be named using >> -the usual reflog syntax (e.g. `stash@{0}` is the most recently >> -created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}` >> -is also possible). >> +The latest stash you created is stored in `refs/stash`; older stashes >> +are found in the reflog of this reference and can be named using the >> +usual reflog syntax (e.g. `stash@{0}` is the most recently created >> +stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}` is also >> +possible). Stashes may also be references by specifying just the stash >> +index (e.g. the integer `n` is equivalent to `stash@{n}`). > > Yay, a documentation update. Should it be s/references/referenced/ in > the second-to-last line? This seems whitespace damaged, though. I see a few at the beginning of lines. Also, Aaron, next time please refrain from reflowing the paragraph unnecessarily. I am guessing that you only added one sentence at the end of an existing paragraph, and such a patch should clearly show that the only change it did is to append at the end. Reflowing will force reviewers to compare the preimage and postimage word by word to spot what other things were changed. > So I don't think this is technically a regression in any > currently-functioning behavior, but it seems like a step in the wrong > direction to add yet another layer of blind parsing. Yes. I agree that the implementation of this patch goes in the wrong direction, even though it means well. >> diff --git a/t/t3907-stash-index.sh b/t/t3907-stash-index.sh >> new file mode 100755 >> index 000..72a1838 >> --- /dev/null >> +++ b/t/t3907-stash-index.sh > > Double yay, tests. > > Do we really need a whole new script for this, though? There are already > "stash show" tests in t3903. We should be able to repeat one of them > using "2" instead of "stash@{2}" (for example). Yes, it seems a lot better direction to go. The existing script t3903 may want to see a bit of modernization clean-up before that to happen, though. Thanks for a review.
Re: [PATCH] stash: allow ref of a stash by index
On Sat, Sep 03, 2016 at 07:21:18PM -0400, Aaron M Watson wrote: > Allows stashes to be referenced by index only. Instead of referencing > "stash@{n}" explicitly, it can simply be referenced as "n". This says "what" but not "why". I assume it is "because the former is more annoying to type". Are there any backwards-compatibility issues you can think of? I think that "123456" could be a sha1, but I do not see much point in referencing a sha1 as the argument of "stash show". And it looks like this code path is called only from is_stash_like(), so presumably the same logic would apply to other callers. So I can't immediately think of any downside to this. > diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt > index 92df596..af11cff 100644 > --- a/Documentation/git-stash.txt > +++ b/Documentation/git-stash.txt > @@ -35,11 +35,12 @@ A stash is by default listed as "WIP on > 'branchname' ...", but > you can give a more descriptive message on the command line when > you create one. > > -The latest stash you created is stored in `refs/stash`; older > -stashes are found in the reflog of this reference and can be named using > -the usual reflog syntax (e.g. `stash@{0}` is the most recently > -created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}` > -is also possible). > +The latest stash you created is stored in `refs/stash`; older stashes > +are found in the reflog of this reference and can be named using the > +usual reflog syntax (e.g. `stash@{0}` is the most recently created > +stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}` is also > +possible). Stashes may also be references by specifying just the stash > +index (e.g. the integer `n` is equivalent to `stash@{n}`). Yay, a documentation update. Should it be s/references/referenced/ in the second-to-last line? > diff --git a/git-stash.sh b/git-stash.sh > index 826af18..a0c7f12 100755 > --- a/git-stash.sh > +++ b/git-stash.sh > @@ -1,4 +1,4 @@ > -#!/bin/sh > +#!/bin/bash I like that you were careful in thinking about bash versus other shells, but this unfortunately isn't going to work. The #!-line here is just a placeholder, and is replaced by the contents of $(SHELL_PATH) by the Makefile. But that's just the mechanical issue. The greater problem is that we can't assume that bash is present at all. So we need to figure out a way to avoid using bash-specific features in the rest of the patch. > @@ -371,6 +371,14 @@ parse_flags_and_rev() > test "$PARSE_CACHE" = "$*" && return 0 # optimisation > PARSE_CACHE="$*" > > + args=() > + for arg > + do > + [[ $arg =~ ^[0-9]+$ ]] && arg="stash@{$arg}" > + args+=("$arg") > + done > + set -- "${args[@]}" > + So here we replace any pure-numeric arguments with stash@{}. I don't think this is quite what we want, as it doesn't take into account where in the parsing we are. When ALLOW_UNKNOWN_FLAGS is set, we might have arbitrary options to pass to "git diff". So: git stash show -l 300 stash@{0} would mistakenly re-write "300" (actually, I think that is already broken because we feed the options to rev-parse which is similarly blind about parsing). Another one is perhaps: git stash show -- 300 ;# only show path "300" of the diff which rev-parse _does_ handle correctly (although we then complain about multiple revs, oblivious to the presence of the "--"). So I don't think this is technically a regression in any currently-functioning behavior, but it seems like a step in the wrong direction to add yet another layer of blind parsing. I wonder if we could do better by making our $FLAGS loop a bit smarter, and stopping at the first non-option (that unfortunately still gets "-l 300" wrong, but is the best we can do, I think). That option becomes $REV, which we can then munge (as your patch does), and feed to "git rev-parse --verify" (which it looks like we already do, though it seems redundant with the earlier call). > + args=() > + for arg > + do > + [[ $arg =~ ^[0-9]+$ ]] && arg="stash@{$arg}" > + args+=("$arg") > + done > + set -- "${args[@]}" Obviously all the array-handling is bash-specific, but that goes away if we just munge the single $REV we find. The "=~" is another bash-ism; it can be replaced with "expr". > diff --git a/t/t3907-stash-index.sh b/t/t3907-stash-index.sh > new file mode 100755 > index 000..72a1838 > --- /dev/null > +++ b/t/t3907-stash-index.sh Double yay, tests. Do we really need a whole new script for this, though? There are already "stash show" tests in t3903. We should be able to repeat one of them using "2" instead of "stash@{2}" (for example). -Peff PS I think this is your first patch. Welcome to the git list. :)
[PATCH] stash: allow ref of a stash by index
Allows stashes to be referenced by index only. Instead of referencing "stash@{n}" explicitly, it can simply be referenced as "n". Signed-off-by: Aaron M Watson --- Documentation/git-stash.txt | 11 --- git-stash.sh| 10 +- t/t3907-stash-index.sh | 77 + 3 files changed, 92 insertions(+), 6 deletions(-) create mode 100755 t/t3907-stash-index.sh diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt index 92df596..af11cff 100644 --- a/Documentation/git-stash.txt +++ b/Documentation/git-stash.txt @@ -35,11 +35,12 @@ A stash is by default listed as "WIP on 'branchname' ...", but you can give a more descriptive message on the command line when you create one. -The latest stash you created is stored in `refs/stash`; older -stashes are found in the reflog of this reference and can be named using -the usual reflog syntax (e.g. `stash@{0}` is the most recently -created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}` -is also possible). +The latest stash you created is stored in `refs/stash`; older stashes +are found in the reflog of this reference and can be named using the +usual reflog syntax (e.g. `stash@{0}` is the most recently created +stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}` is also +possible). Stashes may also be references by specifying just the stash +index (e.g. the integer `n` is equivalent to `stash@{n}`). OPTIONS --- diff --git a/git-stash.sh b/git-stash.sh index 826af18..a0c7f12 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -1,4 +1,4 @@ -#!/bin/sh +#!/bin/bash # Copyright (c) 2007, Nanako Shiraishi dashless=$(basename "$0" | sed -e 's/-/ /') @@ -371,6 +371,14 @@ parse_flags_and_rev() test "$PARSE_CACHE" = "$*" && return 0 # optimisation PARSE_CACHE="$*" + args=() + for arg + do + [[ $arg =~ ^[0-9]+$ ]] && arg="stash@{$arg}" + args+=("$arg") + done + set -- "${args[@]}" + IS_STASH_LIKE= IS_STASH_REF= INDEX_OPTION= diff --git a/t/t3907-stash-index.sh b/t/t3907-stash-index.sh new file mode 100755 index 000..72a1838 --- /dev/null +++ b/t/t3907-stash-index.sh @@ -0,0 +1,77 @@ +#!/bin/sh +# +# Copyright (c) 2016 Aaron M. Watson +# + +test_description='Test git stash with index ref specification' + +. ./test-lib.sh + +test_expect_success 'stash some dirty working directory' ' + echo 1 > file && + git add file && + echo unrelated >other-file && + git add other-file && + test_tick && + git commit -m initial && + echo 2 > file && + git add file && + echo 3 > file && + test_tick && + git stash && + git diff-files --quiet && + git diff-index --cached --quiet HEAD +' + +cat > expect << EOF +diff --git a/file b/file +index 0cfbf08..00750ed 100644 +--- a/file b/file +@@ -1 +1 @@ +-2 ++3 +EOF + +test_expect_success 'applying bogus stash does nothing' ' + test_must_fail git stash apply 1 && + echo 1 >expect && + test_cmp expect file +' + +test_expect_success 'drop middle stash' ' + git reset --hard && + echo 8 > file && + git stash && + echo 9 > file && + git stash && + git stash drop 1 && + test 2 = $(git stash list | wc -l) && + git stash apply && + test 9 = $(cat file) && + test 1 = $(git show :file) && + test 1 = $(git show HEAD:file) && + git reset --hard && + git stash drop && + git stash apply && + test 3 = $(cat file) && + test 1 = $(git show :file) && + test 1 = $(git show HEAD:file) +' + +test_expect_success 'invalid ref of the form "n", where n >= N' ' + git stash clear && + test_must_fail git stash drop 0 && + echo bar5 > file && + echo bar6 > file2 && + git add file2 && + git stash && + test_must_fail git stash drop 1 && + test_must_fail git stash pop 1 && + test_must_fail git stash apply 1 && + test_must_fail git stash show 1 && + test_must_fail git stash branch tmp 1 && + git stash drop +' + +test_done -- 2.7.4