Re: Bug in git-stash(.sh) ?

[Eli Barzilay <eli-oSK4jVRJLyZg9hUCZPvPmw@public.gmane.org>]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 2404 bytes --]

Earlier today, Andreas Schwab wrote:
> 
> FWIW, replacing the spaces by dots will avoid the bug.

(Yeah, but I don't see a quick way to do that replacement without
resorting to bashisms.)


Yesterday, Eli Barzilay wrote:
> I didn't like that either, and I think that it's possible to avoid
> it by dropping the --symbolic for that test, and re-parse if needed
> for an error messag.  I'll try that and send a patch.

I'm attaching a patch rather than including it inline since it's
bigger than I thought, and since I'm not sure that it should be used.
It makes the script use $REV for sha1 revisions, and $SREV for the
--symbolic versions that were used previously.  With this, date refs
work for "stash apply" but they *don't* work for "stash drop" -- looks
like a number is required for that.

However, I thought that a much better solution is to not show the
dates to begin with, since that would make things work as expected...
The fact that things seem to be working fine for the whole world
except for me made me look into my config file, and ...

Three hours ago, Yann Hodique wrote:
> 
> How exactly do you make magit generate these calls?  AFAICT, Magit
> should operate on whatever "git stash list" outputs, meaning
> stash@{N}. So I guess I'm missing something.

... right: the offending configuration I had was log.date = iso.  This
calls for a simple chane for git-stash.sh to use `--date default':

	git log --date default --format="%gd: %gs" -g "$@" $ref_stash --

which follows.  This is independent of the other patch.  In any case,
it is also questionable -- reading the documentation for %gd:

           ·    %gD: reflog selector, e.g., refs/stash@{1}
           ·    %gd: shortened reflog selector, e.g., stash@{1}

makes it look like the problem is there -- in get_reflog_selector() --
which has explicit code for showing the dates.  (This was done in
8f8f5476.)

Another point is being able to see these dates, eg, make "stash list"
show the stash{N} and also show the dates.  It looks to me like the
date code in get_reflog_selector() should be *removed* since it can be
printed with "%cd" or "%ad" in the log line.  And it might be nicer to
add the date to the "stash list" output, something like:

	git log --date default --format="%gd: %gs (%cd)" -g "$@" $ref_stash --


This is the patch mentioned in the beginning:


[-- Attachment #2: .signature --]
[-- Type: text/plain, Size: 151 bytes --]


-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

[-- Attachment #3: srev-patch --]
[-- Type: application/octet-stream, Size: 3347 bytes --]

diff --git a/git-stash.sh b/git-stash.sh
old mode 100755
new mode 100644
index 4e2c7f8..fd59fa6
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -285,9 +285,10 @@ show_stash () {
 # stash records the work tree, and is a merge between the
 # base commit (first parent) and the index tree (second parent).
 #
-#   REV is set to the symbolic version of the specified stash-like commit
-#   IS_STASH_LIKE is non-blank if ${REV} looks like a stash
-#   IS_STASH_REF is non-blank if the ${REV} looks like a stash ref
+#   REV is set to the sha1 of the specified stash-like commit
+#   SREV is set to the its symbolic version
+#   IS_STASH_LIKE is non-blank if ${SREV} looks like a stash
+#   IS_STASH_REF is non-blank if the ${SREV} looks like a stash ref
 #   s is set to the SHA1 of the stash commit
 #   w_commit is set to the commit containing the working tree
 #   b_commit is set to the base commit
@@ -327,8 +328,6 @@ parse_flags_and_rev()
 	i_tree=
 	u_tree=
 
-	REV=$(git rev-parse --no-flags --symbolic "$@") || exit 1
-
 	FLAGS=
 	for opt
 	do
@@ -345,6 +344,9 @@ parse_flags_and_rev()
 		esac
 	done
 
+	REV=$(git rev-parse --no-flags "$@") || exit 1
+	SREV=$(git rev-parse --no-flags --symbolic "$@") || exit 1
+
 	set -- $REV
 
 	case $# in
@@ -356,17 +358,17 @@ parse_flags_and_rev()
 			:
 		;;
 		*)
-			die "$(eval_gettext "Too many revisions specified: \$REV")"
+			die "$(eval_gettext "Too many revisions specified: \$SREV")"
 		;;
 	esac
 
-	REV=$(git rev-parse --quiet --symbolic --verify $1 2>/dev/null) || {
-		reference="$1"
+	reference=$SREV
+	SREV=$(git rev-parse --quiet --symbolic --verify "$SREV" 2>/dev/null) || {
 		die "$(eval_gettext "\$reference is not valid reference")"
 	}
 
-	i_commit=$(git rev-parse --quiet --verify $REV^2 2>/dev/null) &&
-	set -- $(git rev-parse $REV $REV^1 $REV: $REV^1: $REV^2: 2>/dev/null) &&
+	i_commit=$(git rev-parse --quiet --verify "$REV^2" 2>/dev/null) &&
+	set -- $(git rev-parse "$REV" "$REV^1" "$REV:" "$REV^1:" "$REV^2:" 2>/dev/null) &&
 	s=$1 &&
 	w_commit=$1 &&
 	b_commit=$2 &&
@@ -374,11 +376,11 @@ parse_flags_and_rev()
 	b_tree=$4 &&
 	i_tree=$5 &&
 	IS_STASH_LIKE=t &&
-	test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${REV%@*}")" &&
+	test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${SREV%@*}")" &&
 	IS_STASH_REF=t
 
-	u_commit=$(git rev-parse --quiet --verify $REV^3 2>/dev/null) &&
-	u_tree=$(git rev-parse $REV^3: 2>/dev/null)
+	u_commit=$(git rev-parse --quiet --verify "$REV^3" 2>/dev/null) &&
+	u_tree=$(git rev-parse "$REV^3:" 2>/dev/null)
 }
 
 is_stash_like()
@@ -487,9 +489,9 @@ pop_stash() {
 drop_stash () {
 	assert_stash_ref "$@"
 
-	git reflog delete --updateref --rewrite "${REV}" &&
-		say "$(eval_gettext "Dropped \${REV} (\$s)")" ||
-		die "$(eval_gettext "\${REV}: Could not drop stash entry")"
+	git reflog delete --updateref --rewrite "${SREV}" &&
+		say "$(eval_gettext "Dropped \${SREV} (\$s)")" ||
+		die "$(eval_gettext "\${SREV}: Could not drop stash entry")"
 
 	# clear_stash if we just dropped the last stash entry
 	git rev-parse --verify "$ref_stash@{0}" >/dev/null 2>&1 || clear_stash
@@ -503,7 +505,7 @@ apply_to_branch () {
 	set -- --index "$@"
 	assert_stash_like "$@"
 
-	git checkout -b $branch $REV^ &&
+	git checkout -b "$branch" "$REV^" &&
 	apply_stash "$@" && {
 		test -z "$IS_STASH_REF" || drop_stash "$@"
 	}