{"thread":{"id":"43999","subject":"[PATCH] stash: allow ref of a stash by index","startedAt":"2016-09-03T23:21:36Z","lastAt":"2016-09-07T17:50:17Z","messageCount":9,"participants":["Aaron M Watson","Jeff King","Junio C Hamano","Johannes Schindelin","Philip Oakley","Øystein Walle"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"300960","messageId":"1472944878.19860.4.camel@gmail.com","threadId":"43999","inReplyTo":null,"subject":"[PATCH] stash: allow ref of a stash by index","fromName":"Aaron M Watson","fromEmail":"watsona4@gmail.com","sentAt":"2016-09-03T23:21:18Z","receivedAt":"2016-09-03T23:21:36Z","isPatch":true,"sender":{"key":"watsona4@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5954119?v=4"},"body":"Allows stashes to be referenced by index only. Instead of referencing\n\"stash@{n}\" explicitly, it can simply be referenced as \"n\".\n\nSigned-off-by: Aaron M Watson \n---\n Documentation/git-stash.txt | 11 ++++---\n git-stash.sh                | 10 +++++-\n t/t3907-stash-index.sh      | 77\n+++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 92 insertions(+), 6 deletions(-)\n create mode 100755 t/t3907-stash-index.sh\n\ndiff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt\nindex 92df596..af11cff 100644\n--- a/Documentation/git-stash.txt\n+++ b/Documentation/git-stash.txt\n@@ -35,11 +35,12 @@ A stash is by default listed as \"WIP on\n'branchname' ...\", but\n you can give a more descriptive message on the command line when\n you create one.\n \n-The latest stash you created is stored in `refs/stash`; older\n-stashes are found in the reflog of this reference and can be named using\n-the usual reflog syntax (e.g. `stash@{0}` is the most recently\n-created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}`\n-is also possible).\n+The latest stash you created is stored in `refs/stash`; older stashes\n+are found in the reflog of this reference and can be named using the\n+usual reflog syntax (e.g. `stash@{0}` is the most recently created\n+stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}` is also\n+possible). Stashes may also be references by specifying just the stash\n+index (e.g. the integer `n` is equivalent to `stash@{n}`).\n \n OPTIONS\n -------\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 826af18..a0c7f12 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -1,4 +1,4 @@\n-#!/bin/sh\n+#!/bin/bash\n # Copyright (c) 2007, Nanako Shiraishi\n \n dashless=$(basename \"$0\" | sed -e 's/-/ /')\n@@ -371,6 +371,14 @@ parse_flags_and_rev()\n \ttest \"$PARSE_CACHE\" = \"$*\" && return 0 # optimisation\n \tPARSE_CACHE=\"$*\"\n \n+\targs=()\n+\tfor arg\n+\tdo\n+\t\t[[ $arg =~ ^[0-9]+$ ]] && arg=\"stash@{$arg}\"\n+\t\targs+=(\"$arg\")\n+\tdone\n+\tset -- \"${args[@]}\"\n+\n \tIS_STASH_LIKE=\n \tIS_STASH_REF=\n \tINDEX_OPTION=\ndiff --git a/t/t3907-stash-index.sh b/t/t3907-stash-index.sh\nnew file mode 100755\nindex 0000000..72a1838\n--- /dev/null\n+++ b/t/t3907-stash-index.sh\n@@ -0,0 +1,77 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2016 Aaron M. Watson\n+#\n+\n+test_description='Test git stash with index ref specification'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'stash some dirty working directory' '\n+\techo 1 > file &&\n+\tgit add file &&\n+\techo unrelated >other-file &&\n+\tgit add other-file &&\n+\ttest_tick &&\n+\tgit commit -m initial &&\n+\techo 2 > file &&\n+\tgit add file &&\n+\techo 3 > file &&\n+\ttest_tick &&\n+\tgit stash &&\n+\tgit diff-files --quiet &&\n+\tgit diff-index --cached --quiet HEAD\n+'\n+\n+cat > expect << EOF\n+diff --git a/file b/file\n+index 0cfbf08..00750ed 100644\n+--- a/file\n++++ b/file\n+@@ -1 +1 @@\n+-2\n++3\n+EOF\n+\n+test_expect_success 'applying bogus stash does nothing' '\n+\ttest_must_fail git stash apply 1 &&\n+\techo 1 >expect &&\n+\ttest_cmp expect file\n+'\n+\n+test_expect_success 'drop middle stash' '\n+\tgit reset --hard &&\n+\techo 8 > file &&\n+\tgit stash &&\n+\techo 9 > file &&\n+\tgit stash &&\n+\tgit stash drop 1 &&\n+\ttest 2 = $(git stash list | wc -l) &&\n+\tgit stash apply &&\n+\ttest 9 = $(cat file) &&\n+\ttest 1 = $(git show :file) &&\n+\ttest 1 = $(git show HEAD:file) &&\n+\tgit reset --hard &&\n+\tgit stash drop &&\n+\tgit stash apply &&\n+\ttest 3 = $(cat file) &&\n+\ttest 1 = $(git show :file) &&\n+\ttest 1 = $(git show HEAD:file)\n+'\n+\n+test_expect_success 'invalid ref of the form \"n\", where n >= N' '\n+\tgit stash clear &&\n+\ttest_must_fail git stash drop 0 &&\n+\techo bar5 > file &&\n+\techo bar6 > file2 &&\n+\tgit add file2 &&\n+\tgit stash &&\n+\ttest_must_fail git stash drop 1 &&\n+\ttest_must_fail git stash pop 1 &&\n+\ttest_must_fail git stash apply 1 &&\n+\ttest_must_fail git stash show 1 &&\n+\ttest_must_fail git stash branch tmp 1 &&\n+\tgit stash drop\n+'\n+\n+test_done\n-- \n2.7.4\n"},{"id":"300961","messageId":"20160904015209.ba6arov46ntr2ouq@sigill.intra.peff.net","threadId":"43999","inReplyTo":"1472944878.19860.4.camel@gmail.com","subject":"Re: [PATCH] stash: allow ref of a stash by index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-04T01:52:09Z","receivedAt":"2016-09-04T01:52:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 03, 2016 at 07:21:18PM -0400, Aaron M Watson wrote:\n\n> Allows stashes to be referenced by index only. Instead of referencing\n> \"stash@{n}\" explicitly, it can simply be referenced as \"n\".\n\nThis says \"what\" but not \"why\". I assume it is \"because the former is\nmore annoying to type\".\n\nAre there any backwards-compatibility issues you can think of?\n\nI think that \"123456\" could be a sha1, but I do not see much point in\nreferencing a sha1 as the argument of \"stash show\". And it looks like\nthis code path is called only from is_stash_like(), so presumably the\nsame logic would apply to other callers.\n\nSo I can't immediately think of any downside to this.\n\n> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt\n> index 92df596..af11cff 100644\n> --- a/Documentation/git-stash.txt\n> +++ b/Documentation/git-stash.txt\n> @@ -35,11 +35,12 @@ A stash is by default listed as \"WIP on\n> 'branchname' ...\", but\n>  you can give a more descriptive message on the command line when\n>  you create one.\n>  \n> -The latest stash you created is stored in `refs/stash`; older\n> -stashes are found in the reflog of this reference and can be named using\n> -the usual reflog syntax (e.g. `stash@{0}` is the most recently\n> -created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}`\n> -is also possible).\n> +The latest stash you created is stored in `refs/stash`; older stashes\n> +are found in the reflog of this reference and can be named using the\n> +usual reflog syntax (e.g. `stash@{0}` is the most recently created\n> +stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}` is also\n> +possible). Stashes may also be references by specifying just the stash\n> +index (e.g. the integer `n` is equivalent to `stash@{n}`).\n\nYay, a documentation update. Should it be s/references/referenced/ in\nthe second-to-last line?\n\n> diff --git a/git-stash.sh b/git-stash.sh\n> index 826af18..a0c7f12 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -1,4 +1,4 @@\n> -#!/bin/sh\n> +#!/bin/bash\n\nI like that you were careful in thinking about bash versus other shells,\nbut this unfortunately isn't going to work. The #!-line here is just a\nplaceholder, and is replaced by the contents of $(SHELL_PATH) by the\nMakefile.\n\nBut that's just the mechanical issue. The greater problem is that we\ncan't assume that bash is present at all. So we need to figure out a way\nto avoid using bash-specific features in the rest of the patch.\n\n> @@ -371,6 +371,14 @@ parse_flags_and_rev()\n>  \ttest \"$PARSE_CACHE\" = \"$*\" && return 0 # optimisation\n>  \tPARSE_CACHE=\"$*\"\n>  \n> +\targs=()\n> +\tfor arg\n> +\tdo\n> +\t\t[[ $arg =~ ^[0-9]+$ ]] && arg=\"stash@{$arg}\"\n> +\t\targs+=(\"$arg\")\n> +\tdone\n> +\tset -- \"${args[@]}\"\n> +\n\nSo here we replace any pure-numeric arguments with stash@{}. I don't\nthink this is quite what we want, as it doesn't take into account where\nin the parsing we are. When ALLOW_UNKNOWN_FLAGS is set, we might have\narbitrary options to pass to \"git diff\". So:\n\n git stash show -l 300 stash@{0}\n\nwould mistakenly re-write \"300\" (actually, I think that is already\nbroken because we feed the options to rev-parse which is similarly blind\nabout parsing).\n\nAnother one is perhaps:\n\n git stash show -- 300 ;# only show path \"300\" of the diff\n\nwhich rev-parse _does_ handle correctly (although we then complain about\nmultiple revs, oblivious to the presence of the \"--\").\n\nSo I don't think this is technically a regression in any\ncurrently-functioning behavior, but it seems like a step in the wrong\ndirection to add yet another layer of blind parsing.\n\nI wonder if we could do better by making our $FLAGS loop a bit smarter,\nand stopping at the first non-option (that unfortunately still gets \"-l\n300\" wrong, but is the best we can do, I think). That option becomes\n$REV, which we can then munge (as your patch does), and feed to \"git\nrev-parse --verify\" (which it looks like we already do, though it seems\nredundant with the earlier call).\n\n\n> +\targs=()\n> +\tfor arg\n> +\tdo\n> +\t\t[[ $arg =~ ^[0-9]+$ ]] && arg=\"stash@{$arg}\"\n> +\t\targs+=(\"$arg\")\n> +\tdone\n> +\tset -- \"${args[@]}\"\n\nObviously all the array-handling is bash-specific, but that goes away if\nwe just munge the single $REV we find.\n\nThe \"=~\" is another bash-ism; it can be replaced with \"expr\".\n\n> diff --git a/t/t3907-stash-index.sh b/t/t3907-stash-index.sh\n> new file mode 100755\n> index 0000000..72a1838\n> --- /dev/null\n> +++ b/t/t3907-stash-index.sh\n\nDouble yay, tests.\n\nDo we really need a whole new script for this, though? There are already\n\"stash show\" tests in t3903. We should be able to repeat one of them\nusing \"2\" instead of \"stash@{2}\" (for example).\n\n-Peff\n\nPS I think this is your first patch. Welcome to the git list. :)\n"},{"id":"300969","messageId":"xmqqd1kkqhb0.fsf@gitster.mtv.corp.google.com","threadId":"43999","inReplyTo":"20160904015209.ba6arov46ntr2ouq@sigill.intra.peff.net","subject":"Re: [PATCH] stash: allow ref of a stash by index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-04T07:01:55Z","receivedAt":"2016-09-04T07:02:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt\n>> index 92df596..af11cff 100644\n>> --- a/Documentation/git-stash.txt\n>> +++ b/Documentation/git-stash.txt\n>> @@ -35,11 +35,12 @@ A stash is by default listed as \"WIP on\n>> 'branchname' ...\", but\n>>  you can give a more descriptive message on the command line when\n>>  you create one.\n>>  \n>> -The latest stash you created is stored in `refs/stash`; older\n>> -stashes are found in the reflog of this reference and can be named using\n>> -the usual reflog syntax (e.g. `stash@{0}` is the most recently\n>> -created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}`\n>> -is also possible).\n>> +The latest stash you created is stored in `refs/stash`; older stashes\n>> +are found in the reflog of this reference and can be named using the\n>> +usual reflog syntax (e.g. `stash@{0}` is the most recently created\n>> +stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}` is also\n>> +possible). Stashes may also be references by specifying just the stash\n>> +index (e.g. the integer `n` is equivalent to `stash@{n}`).\n>\n> Yay, a documentation update. Should it be s/references/referenced/ in\n> the second-to-last line?\n\nThis seems whitespace damaged, though. I see a few   at the\nbeginning of lines.\n\nAlso, Aaron, next time please refrain from reflowing the paragraph\nunnecessarily. I am guessing that you only added one sentence at\nthe end of an existing paragraph, and such a patch should clearly\nshow that the only change it did is to append at the end. Reflowing\nwill force reviewers to compare the preimage and postimage word by\nword to spot what other things were changed.\n\n> So I don't think this is technically a regression in any\n> currently-functioning behavior, but it seems like a step in the wrong\n> direction to add yet another layer of blind parsing.\n\nYes. I agree that the implementation of this patch goes in the\nwrong direction, even though it means well.\n\n>> diff --git a/t/t3907-stash-index.sh b/t/t3907-stash-index.sh\n>> new file mode 100755\n>> index 0000000..72a1838\n>> --- /dev/null\n>> +++ b/t/t3907-stash-index.sh\n>\n> Double yay, tests.\n>\n> Do we really need a whole new script for this, though? There are already\n> \"stash show\" tests in t3903. We should be able to repeat one of them\n> using \"2\" instead of \"stash@{2}\" (for example).\n\nYes, it seems a lot better direction to go. The existing script\nt3903 may want to see a bit of modernization clean-up before that to\nhappen, though.\n\nThanks for a review.\n\n"},{"id":"300970","messageId":"alpine.DEB.2.20.1609040914200.129229@virtualbox","threadId":"43999","inReplyTo":"20160904015209.ba6arov46ntr2ouq@sigill.intra.peff.net","subject":"Re: [PATCH] stash: allow ref of a stash by index","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-04T07:21:22Z","receivedAt":"2016-09-04T07:21:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 3 Sep 2016, Jeff King wrote:\n\n> On Sat, Sep 03, 2016 at 07:21:18PM -0400, Aaron M Watson wrote:\n> \n> > Allows stashes to be referenced by index only. Instead of referencing\n> > \"stash@{n}\" explicitly, it can simply be referenced as \"n\".\n> \n> This says \"what\" but not \"why\". I assume it is \"because the former is\n> more annoying to type\".\n> \n> Are there any backwards-compatibility issues you can think of?\n> \n> I think that \"123456\" could be a sha1, but I do not see much point in\n> referencing a sha1 as the argument of \"stash show\". And it looks like\n> this code path is called only from is_stash_like(), so presumably the\n> same logic would apply to other callers.\n\nMaybe we could make it unambiguous, e.g. by using # instead: #123456\ncannot refer to a SHA-1.\n\nBut then, '#' are comment-starting in shells, so they would have to by\nescaped. Maybe the best option would be to introduce a -n option,\nwith the shortcut - thanks to e0319ff (parseopt: add\nOPT_NUMBER_CALLBACK, 2009-05-07).\n\nCiao,\nJohannes\n"},{"id":"300982","messageId":"B8553589D87B4CFAB94CFB22027B9E7D@PhilipOakley","threadId":"43999","inReplyTo":"alpine.DEB.2.20.1609040914200.129229@virtualbox","subject":"Re: [PATCH] stash: allow ref of a stash by index","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2016-09-04T10:57:51Z","receivedAt":"2016-09-04T10:57:56Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Johannes Schindelin\" \n> Hi,\n>\n> On Sat, 3 Sep 2016, Jeff King wrote:\n>\n>> On Sat, Sep 03, 2016 at 07:21:18PM -0400, Aaron M Watson wrote:\n>>\n>> > Allows stashes to be referenced by index only. Instead of referencing\n>> > \"stash@{n}\" explicitly, it can simply be referenced as \"n\".\n>>\n>> This says \"what\" but not \"why\". I assume it is \"because the former is\n>> more annoying to type\".\n>>\n>> Are there any backwards-compatibility issues you can think of?\n>>\n>> I think that \"123456\" could be a sha1, but I do not see much point in\n>> referencing a sha1 as the argument of \"stash show\". And it looks like\n>> this code path is called only from is_stash_like(), so presumably the\n>> same logic would apply to other callers.\n>\n> Maybe we could make it unambiguous, e.g. by using # instead: #123456\n> cannot refer to a SHA-1.\n\nThe alternative is to limit the length to less that the shortest ambiguous \nsha1 length that has been used (by Git users - 5, 6, 7? )? Which is probably \nallowing 1-4 characters, which is a reasonbly deep stash index...\n\nIf you need to refer to stash@{9362} you have bigger problems.\n\n> But then, '#' are comment-starting in shells, so they would have to by\n> escaped. Maybe the best option would be to introduce a -n option,\n> with the shortcut - thanks to e0319ff (parseopt: add\n> OPT_NUMBER_CALLBACK, 2009-05-07).\n>\n> Ciao,\n> Johannes\n> \n\n"},{"id":"301142","messageId":"CAFaJEqu-JUcwLjrQBk_huSa3DZfCf8O4eAZ=UgcXHzN=CLgtpw@mail.gmail.com","threadId":"43999","inReplyTo":"20160904015209.ba6arov46ntr2ouq@sigill.intra.peff.net","subject":"Re: [PATCH] stash: allow ref of a stash by index","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2016-09-05T21:46:34Z","receivedAt":"2016-09-05T21:47:21Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"Hi, guys\n\nI like this idea. It makes stash easier and quicker to use, and it\n\"hides\" the fact that it uses the reflog for keeping track of the made\nstashes. *Not* to say I discourage interested people from peeking under\nthe hood. I just think it's nice to sometimes think of the stash as a\nseparate concept instead of being built on top of strange merge\ncommits constructed in temporary indexes :)\n\nThe bash-specific code is a no-go, so here's a way to do it in a way\nthat I think is in line with Git's code style for shell scripts. I took\nthe liberty of removing the '|| exit 1' since the rev is verified later\non anyway, as can be seen in the last piece of context. That way the\nargument munging can be done at a later stage where we don't have to\nloop over multiple ones. The first rev-parse's purpose is just to apply\n--sq.\n\n(Besides, the only way to do it at the top like in the original patch\nwas for arg in bleh \"$@\" where bleh is a marker to indicate that the\npositional arguments should be cleared in a separate case branch so that\nthey can be rebuilt with multiple invocations of set later. Not pretty,\nin my opinion.)\n\nRegards,\nØsse\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 826af18..b026288 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -384,7 +384,7 @@ parse_flags_and_rev()\n i_tree=\n u_tree=\n\n- REV=$(git rev-parse --no-flags --symbolic --sq \"$@\") || exit 1\n+ REV=$(git rev-parse --no-flags --symbolic --sq \"$@\" 2>/dev/null)\n\n FLAGS=\n for opt\n@@ -422,6 +422,15 @@ parse_flags_and_rev()\n ;;\n esac\n\n+ case \"$1\" in\n+ *[!0-9]*)\n+ :\n+ ;;\n+ *)\n+ set -- \"${ref_stash}@{$1}\"\n+ ;;\n+ esac\n+\n REV=$(git rev-parse --symbolic --verify --quiet \"$1\") || {\n reference=\"$1\"\n die \"$(eval_gettext \"\\$reference is not a valid reference\")\"\n"},{"id":"301150","messageId":"CAFaJEqv-eZaRwrZhq=rNzQ4gEqE3WXwZVrF61opk-Z2=RrxcbQ@mail.gmail.com","threadId":"43999","inReplyTo":"CAFaJEqu-JUcwLjrQBk_huSa3DZfCf8O4eAZ=UgcXHzN=CLgtpw@mail.gmail.com","subject":"Re: [PATCH] stash: allow ref of a stash by index","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2016-09-05T21:58:31Z","receivedAt":"2016-09-05T21:59:19Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"Pasting text into Gmail's web interface is not conducive to sending tabs\nthrough the intertubes. But you get the idea..\n\nØsse\n"},{"id":"301155","messageId":"20160905235207.jkxmnrwoc6lllomk@sigill.intra.peff.net","threadId":"43999","inReplyTo":"CAFaJEqu-JUcwLjrQBk_huSa3DZfCf8O4eAZ=UgcXHzN=CLgtpw@mail.gmail.com","subject":"Re: [PATCH] stash: allow ref of a stash by index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-05T23:52:08Z","receivedAt":"2016-09-05T23:52:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 05, 2016 at 11:46:34PM +0200, Øystein Walle wrote:\n\n> The bash-specific code is a no-go, so here's a way to do it in a way\n> that I think is in line with Git's code style for shell scripts. I took\n> the liberty of removing the '|| exit 1' since the rev is verified later\n> on anyway, as can be seen in the last piece of context. That way the\n> argument munging can be done at a later stage where we don't have to\n> loop over multiple ones. The first rev-parse's purpose is just to apply\n> --sq.\n\nI wondered how that would impact the error message when there is no such\nstash. It looks like rev-parse will still return the bogus name on\nstdout, so we do not run afoul of the \"No stash found\" code path. We do\nget:\n\n $ git.compile stash show foobar\n foobar is not a valid reference\n\ninstead of:\n\n $ git stash show foobar\n fatal: ambiguous argument 'foobar': unknown revision or path not in\n the working tree.\n Use '--' to separate paths from revisions, like this:\n 'git [...] -- [...]'\n\nBut I do not see that as a big downside (I might even call it an\nimprovement).\n\n-Peff\n"},{"id":"301299","messageId":"xmqqbmzzoazy.fsf@gitster.mtv.corp.google.com","threadId":"43999","inReplyTo":"CAFaJEqu-JUcwLjrQBk_huSa3DZfCf8O4eAZ=UgcXHzN=CLgtpw@mail.gmail.com","subject":"Re: [PATCH] stash: allow ref of a stash by index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-07T17:50:09Z","receivedAt":"2016-09-07T17:50:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Øystein Walle writes:\n\n> diff --git a/git-stash.sh b/git-stash.sh\n> index 826af18..b026288 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -384,7 +384,7 @@ parse_flags_and_rev()\n> i_tree=\n> u_tree=\n>\n> - REV=$(git rev-parse --no-flags --symbolic --sq \"$@\") || exit 1\n> + REV=$(git rev-parse --no-flags --symbolic --sq \"$@\" 2>/dev/null)\n>\n> FLAGS=\n> for opt\n> @@ -422,6 +422,15 @@ parse_flags_and_rev()\n> ;;\n> esac\n>\n> + case \"$1\" in\n> + *[!0-9]*)\n> + :\n\nOK, so you ignore anything that has a non-digit here, to ensure that...\n\n> + ;;\n> + *)\n> + set -- \"${ref_stash}@{$1}\"\n\n... this one triggers only for a string $1 that consists solely of\ndigits.\n\n> + ;;\n> + esac\n\nMakes sense. I notice that both of the two existing case/esac\nstatements in this function indent case arms and their bodies one\nlevel too deep, which you follwed with the above addition. That may\nbe something we would want to fix in a follow-up patch.\n\n> REV=$(git rev-parse --symbolic --verify --quiet \"$1\") || {\n> reference=\"$1\"\n> die \"$(eval_gettext \"\\$reference is not a valid reference\")\"\n\nI agree with Peff that the change in error message he noticed is\nprobably an improvement ;-)\n\nWant to do a final log message to make it a real patch?\n\nThanks.\n"}]}