Re: [PATCH (3rd try)] Add git-stash script

Re: [PATCH (3rd try)] Add git-stash script

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2267 bytes --]

Hi,

On Sun, 1 Jul 2007, しらいしななこ wrote:

> Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> 
> > On Sat, 30 Jun 2007, しらいしななこ wrote:
> >
> >> diff --git a/git-stash.sh b/git-stash.sh
> >> [...]
> >> +	printf >&2 'Saved WIP on %s\n' "$msg"
> >
> > You have an awful lot of printfs in the code. Why not just use echos?
> 
> I just imitated other scripts.  I can change it to
> 
> echo >&2 "Saved WIP on $msg"
> 
> but after reading Junio's comments, I think I probably should not.

You can leave them as-are, but I am actually more used to reading 
something like

	echo "Saved WIP on $msg" >&2

> >> +list_stash () {
> >> +	git-log --pretty=oneline -g "$@" $ref_stash |
> >
> > Wouldn't you want "--default $ref_stash" here?
> 
> I do not know, and I'm sorry I do not understand Junio's comments.
> 
> What does --default do in this case?

I had the impression that

	git stash list stash@{3}

would make sense. Probably I was mistaken.

> >> +apply_stash () {
> >> +	git-diff-files --quiet ||
> >> +		die 'Cannot restore on top of a dirty state'
> >
> > You meant "no_changes", right? I think you miss changes in the index 
> > otherwise.
> 
> After I read exchanges between you and Junio I do not know which way is 
> preferred.

Well, there are two differing, and contradicting, preferences: Junio's and 
mine.

Now you can have a third preference, but in the end what weighs most is 
the opinion of the person implementing it.

> Using no_changes does not forbid me from doing that, but I think Junio's 
> example will be forbidden.  The original scenario was that I apply a 
> stashed change to an unmodified state, and there is no problem either 
> way.

Oh, but there is. Imagine this:

File "f" contains "1" in HEAD, "2" in the index and "3" in the working 
directory. Now you say "git stash". Okay, file "f" now contains "1" in all 
three, and the working directory is clean. Suppose you decide the stash 
was a mistake. "git stash apply". File "f" contains "1" in _both_ the HEAD 
and the index, and "3" in the working directory.

Now, you might say that it does not matter. But then we should not even 
save the index.

Can I make one further request? Add "git stash save" to do the same as 
"git stash"?

Ciao,
Dscho

Re: [PATCH (2nd try)] Add git-stash script

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 306 bytes --]

Hi,

On Sat, 30 Jun 2007, しらいしななこ wrote:

> Unfortunately I haven't managed to get the suggestion to use "export 
> GITHEAD_xxxx=NicerName" from Johannes Schindelin working yet.

If you provide a test script (see t/t[0-9]*.sh), I'll gladly provide the 
support for GITHEAD_xxxx.

Ciao,
Dscho

[PATCH (3rd try)] Add git-stash script

[しらいしななこ]

When my boss has something to show me and I have to update, for some
reason I am always in the middle of doing something else, and git pull
command refuses to work in such a case.

I wrote this little script to save the changes I made, perform the
update, and then come back to where I was, but on top of the updated
commit.

This is how you would use the script:

  $ git stash
  $ git pull
  $ git stash apply

Signed-off-by: Nanako Shiraishi <nanako3@bluebottle.com>
---

Thank you for the hint for labeling the conflict blocks.
I also added an entry to gitignore and Makefile as requested.

 .gitignore   |    1 +
 Makefile     |    3 +-
 git-stash.sh |  160 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 163 insertions(+), 1 deletions(-)
 create mode 100755 git-stash.sh

diff --git a/.gitignore b/.gitignore
index e8b060c..02d9b04 100644
--- a/.gitignore
+++ b/.gitignore
@@ -123,6 +123,7 @@ git-ssh-fetch
 git-ssh-pull
 git-ssh-push
 git-ssh-upload
+git-stash
 git-status
 git-stripspace
 git-submodule
diff --git a/Makefile b/Makefile
index a98e27a..05b1fc0 100644
--- a/Makefile
+++ b/Makefile
@@ -212,7 +212,8 @@ SCRIPT_SH = \
 	git-merge.sh git-merge-stupid.sh git-merge-octopus.sh \
 	git-merge-resolve.sh git-merge-ours.sh \
 	git-lost-found.sh git-quiltimport.sh git-submodule.sh \
-	git-filter-branch.sh
+	git-filter-branch.sh \
+	git-stash.sh
 
 SCRIPT_PERL = \
 	git-add--interactive.perl \
diff --git a/git-stash.sh b/git-stash.sh
new file mode 100755
index 0000000..05ca3a6
--- /dev/null
+++ b/git-stash.sh
@@ -0,0 +1,160 @@
+#!/bin/sh
+# Copyright (c) 2007, Nanako Shiraishi
+
+USAGE='[ | list | show | apply | clear]'
+
+. git-sh-setup
+require_work_tree
+
+TMP="$GIT_DIR/.git-stash.$$"
+trap 'rm -f "$TMP-*"' 0
+
+ref_stash=refs/stash
+
+no_changes () {
+	git-diff-index --quiet --cached HEAD &&
+	git-diff-files --quiet
+}
+
+clear_stash () {
+	logfile="$GIT_DIR/logs/$ref_stash" &&
+	mkdir -p "$(dirname "$logfile")" &&
+	: >"$logfile"
+}
+
+save_stash () {
+	if no_changes
+	then
+		echo >&2 'No local changes to save'
+		exit 0
+	fi
+	test -f "$GIT_DIR/logs/refs/stash" ||
+		clear_stash || die "Cannot initialize stash"
+
+	# state of the base commit
+	if b_commit=$(git-rev-parse --verify HEAD)
+	then
+		head=$(git-log --abbrev-commit --pretty=oneline -n 1 HEAD)
+	else
+		die "You do not have the initial commit yet"
+	fi
+
+	if branch=$(git-symbolic-ref -q HEAD)
+	then
+		branch=${branch#refs/heads/}
+	else
+		branch='(no branch)'
+	fi
+	msg=$(printf '%s: %s' "$branch" "$head")
+
+	# state of the index
+	i_tree=$(git-write-tree) &&
+	i_commit=$(printf 'index on %s' "$msg" |
+		git-commit-tree $i_tree -p $b_commit) ||
+		die "Cannot save the current index state"
+
+	# state of the working tree
+	w_tree=$( (
+		GIT_INDEX_FILE="$TMP-index" &&
+		export GIT_INDEX_FILE &&
+
+		rm -f "$TMP-index" &&
+		git-read-tree $i_tree &&
+		git-add -u &&
+		git-write-tree &&
+		rm -f "$TMP-index"
+	) ) ||
+		die "Cannot save the current worktree state"
+
+	# create the stash
+	w_commit=$(printf 'WIP on %s' "$msg" |
+		git-commit-tree $w_tree -p $b_commit -p $i_commit) ||
+		die "Cannot record working tree state"
+
+	git-update-ref -m "$msg" $ref_stash $w_commit ||
+		die "Cannot save the current status"
+	printf >&2 'Saved WIP on %s\n' "$msg"
+}
+
+list_stash () {
+	git-log --pretty=oneline -g "$@" $ref_stash |
+	sed -n -e 's/^[.0-9a-f]* refs\///p'
+}
+
+show_stash () {
+	flags=$(git-rev-parse --no-revs --flags "$@")
+	if test -z "$flags"
+	then
+		flags=--stat
+	fi
+	s=$(git-rev-parse --revs-only --no-flags --default $ref_stash "$@")
+
+	w_commit=$(git-rev-parse --verify "$s") &&
+	b_commit=$(git-rev-parse --verify "$s^") &&
+	git-diff $flags $b_commit $w_commit
+}
+
+apply_stash () {
+	git-diff-files --quiet ||
+		die 'Cannot restore on top of a dirty state'
+
+	# current index state
+	c_tree=$(git-write-tree) ||
+		die 'Cannot apply a stash in the middle of a merge'
+
+	s=$(git-rev-parse --revs-only --no-flags --default $ref_stash "$@") &&
+	w_tree=$(git-rev-parse --verify "$s:") &&
+	b_tree=$(git-rev-parse --verify "$s^:") ||
+		die "$*: no valid stashed state found"
+
+	eval "
+		GITHEAD_$w_tree='Stashed changes' &&
+		GITHEAD_$c_tree='Updated upstream' &&
+		GITHEAD_$b_tree='Version stash was based on' &&
+		export GITHEAD_$w_tree GITHEAD_$c_tree GITHEAD_$b_tree
+	"
+
+	if git-merge-recursive $b_tree -- $c_tree $w_tree
+	then
+		# No conflict
+		a="$TMP-added" &&
+		git-diff --cached --name-only --diff-filter=A $c_tree >"$a" &&
+		git-read-tree --reset $c_tree &&
+		git-update-index --add --stdin <"$a" ||
+			die "Cannot unstage modified files"
+		git-status
+		rm -f "$a"
+	else
+		# Merge conflict
+		exit 1
+	fi
+}
+
+# Main command set
+case "$1" in
+list)
+	shift
+	if test $# = 0
+	then
+		set x -n 10
+		shift
+	fi
+	list_stash "$@"
+	;;
+show)
+	shift
+	show_stash "$@"
+	;;
+apply)
+	shift
+	apply_stash "$@"
+	;;
+clear)
+	clear_stash
+	;;
+'')
+	save_stash && git-reset --hard
+	;;
+*)
+	usage
+esac
-- 
1.5.2

----------------------------------------------------------------------
Free pop3 email with a spam filter.
http://www.bluebottle.com

Re: [PATCH (3rd try)] Add git-stash script

[Jeff King]

On Sat, Jun 30, 2007 at 02:37:09PM +0900, しらいしななこ wrote:

> +ref_stash=refs/stash
[...]
> +save_stash () {
> +	if no_changes
> +	then
> +		echo >&2 'No local changes to save'
> +		exit 0
> +	fi
> +	test -f "$GIT_DIR/logs/refs/stash" ||
> +		clear_stash || die "Cannot initialize stash"

Nit: this should be .../logs/$ref_stash

-Peff

Re: [PATCH (3rd try)] Add git-stash script

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Jun 30, 2007 at 02:37:09PM +0900, しらいしななこ wrote:
>
>> +ref_stash=refs/stash
> [...]
>> +save_stash () {
>> +	if no_changes
>> +	then
>> +		echo >&2 'No local changes to save'
>> +		exit 0
>> +	fi
>> +	test -f "$GIT_DIR/logs/refs/stash" ||
>> +		clear_stash || die "Cannot initialize stash"
>
> Nit: this should be .../logs/$ref_stash

Sharp eyes.

I'll take a look and possibly comment on it later, but from a
cursory look I do not see major problems, so if I decide to
apply it I'll try to remember fixing this up when I do so...

Re: [PATCH (3rd try)] Add git-stash script

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1104 bytes --]

Hi,

On Sat, 30 Jun 2007, しらいしななこ wrote:

> diff --git a/git-stash.sh b/git-stash.sh
> [...]
> +	printf >&2 'Saved WIP on %s\n' "$msg"

You have an awful lot of printfs in the code. Why not just use echos?

> +list_stash () {
> +	git-log --pretty=oneline -g "$@" $ref_stash |

Wouldn't you want "--default $ref_stash" here?

> +apply_stash () {
> +	git-diff-files --quiet ||
> +		die 'Cannot restore on top of a dirty state'

You meant "no_changes", right? I think you miss changes in the index 
otherwise.

> +		git-diff --cached --name-only --diff-filter=A $c_tree >"$a" &&
> +		git-read-tree --reset $c_tree &&
> +		git-update-index --add --stdin <"$a" ||
> +			die "Cannot unstage modified files"

Isn't there a way to avoid the temporary file here?

> +	else
> +		# Merge conflict
> +		exit 1

Since $? is already != 0, and it might tell the savvy user what kind of 
error merge-recursive returned, why not use "exit", which is equivalent to 
"exit $?"?

> +		set x -n 10
> +		shift

This is more elegantly written as "set -- -n 10", or in our context even 
"set -- -10".

Ciao,
Dscho

Re: [PATCH (3rd try)] Add git-stash script

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> diff --git a/git-stash.sh b/git-stash.sh
>> [...]
>> +	printf >&2 'Saved WIP on %s\n' "$msg"
>
> You have an awful lot of printfs in the code. Why not just use echos?

I had the same impression, but I would say it is Ok (actually
printf is even better).  Judging from our recent changes
(e.g. a23bfae) I think it is prudent to avoid echo when printing
user supplied messages.  Not that corrupted backslash sequence
matters that much on stderr, though ;-).

>> +list_stash () {
>> +	git-log --pretty=oneline -g "$@" $ref_stash |
>
> Wouldn't you want "--default $ref_stash" here?

But "git log --pretty=oneline -g master --default refs/stash"
would not make much sense would it?  The design currently seems
to follow what I suggested earlier, namely to use a single stash
ref per repository, so $ref_stash is spelled as a variable but
it is a constant.  IOW, you do not specify "alternate stash"
from the command line.

Unfortunately we do not have a good way to reject non-flag rev
parameters in "$@" to error out "git stash list master".  What
we would want here is a way to allow things like --since=1.hour
and -20 while rejecting branch/tag names and other parameters.

>> +apply_stash () {
>> +	git-diff-files --quiet ||
>> +		die 'Cannot restore on top of a dirty state'
>
> You meant "no_changes", right? I think you miss changes in the index 
> otherwise.

Interestingly, I think this is actually correct in the sense
that the code is internally consistent, as it uses the current
index, not the HEAD, as the base of application.  It however is
debatable if this sequence (which is allowed because it does not
use no_changes here) makes sense:

	: hack hack hack
	: get interrupted
	$ git stash
	: do something else on a clean slate
	$ git commit
        : hack hack hack
        $ git add -u
        $ git unstash

>> +		git-diff --cached --name-only --diff-filter=A $c_tree >"$a" &&
>> +		git-read-tree --reset $c_tree &&
>> +		git-update-index --add --stdin <"$a" ||
>> +			die "Cannot unstage modified files"
>
> Isn't there a way to avoid the temporary file here?

We could obviously do

	files=$(git-diff --cached ...) &&
        git-read-tree --reset $c_tree &&
        echo "$files" | git-update-index --add --stdin

Oops, the last "echo" may have to be "printf '%s'" ;-)

There might be quite many files that have been added to make the
$files variable too big to be given to echo, though.

	git-diff --cached ... |
        (
        	git-read-tree --reset $c_tree &&
                git-update-index --add --stdin
	)

would not work, unless you copy the index into a temporary file
and have the upstream git-diff use it.  But then we are using a
temporary file anyway.

>> +		set x -n 10
>> +		shift
>
> This is more elegantly written as "set -- -n 10", or in our context even 
> "set -- -10".

That is Ok in POSIX only world, but so far we have stayed away
from using "set -- potentially-dangerous-string" in any of our
scripts.  I would feel x + shift is safer and more comfortable
but probably that is largely because I am old fashioned.

Re: [PATCH (3rd try)] Add git-stash script

[しらいしななこ]

Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:

> Hi,

Hello.

> On Sat, 30 Jun 2007, しらいしななこ wrote:
>
>> diff --git a/git-stash.sh b/git-stash.sh
>> [...]
>> +	printf >&2 'Saved WIP on %s\n' "$msg"
>
> You have an awful lot of printfs in the code. Why not just use echos?

I just imitated other scripts.  I can change it to

echo >&2 "Saved WIP on $msg"

but after reading Junio's comments, I think I probably should not.

>> +list_stash () {
>> +	git-log --pretty=oneline -g "$@" $ref_stash |
>
> Wouldn't you want "--default $ref_stash" here?

I do not know, and I'm sorry I do not understand Junio's comments.

What does --default do in this case?

>> +apply_stash () {
>> +	git-diff-files --quiet ||
>> +		die 'Cannot restore on top of a dirty state'
>
> You meant "no_changes", right? I think you miss changes in the index 
> otherwise.

After I read exchanges between you and Junio I do not know which way
is preferred.  Using no_changes does not forbid me from doing that,
but I think Junio's example will be forbidden.  The original scenario
was that I apply a stashed change to an unmodified state, and there is
no problem either way.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

----------------------------------------------------------------------
Free pop3 email with a spam filter.
http://www.bluebottle.com