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

On Sat, Sep 03, 2016 at 07:21:18PM -0400, Aaron M Watson wrote:

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.

Yay, a documentation update. Should it be s/references/referenced/ in the second-to-last line?

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.

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

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

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