Re: [PATCH] stash: allow ref of a stash by index

[Junio C Hamano]

Ø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

[Jeff King]

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

[Øystein Walle]

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

[Øystein Walle]

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

[Philip Oakley]

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

[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.

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

[Junio C Hamano]

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

[Jeff King]

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. :)