Re: [PATCH] Add git-save script

Nanako Shiraishi [off-list ref] writes:

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.

Here is how to use my script:

    $ git save
    $ git pull
    $ git save restore

Heh, I earlier said that I'd start looking at the "stash", but
haven't done anything about it so far (I was busy with the day
job).  More honestly, I was being lazy, and sometimes lazy are
rewarded, with a contribution from a new git hacker.

I think this is more or less in the direction I was planning to
head to; thanks for the patch.

Having said that, I have a few comments.  Style, robustness,
and portability.

Style.  "#!/bin/sh" (no space after she-bang).

+
+. git-sh-setup
+require_work_tree
+
+function save_work () {

No noiseword "function" for portability.

When you say "#!/bin/sh", you are writing for the family of
generic Bourne shells, not specifically for korn nor bash.  For
example, dash is a fine POSIX shell, but does not grok function
noiseword.  When in doubt, please stay away from things not in
POSIX (e.g. function, [[, ]], ${parameter//pattern/string/}).

+	save=$( (
+		i_tree=$(git-write-tree)
+
+		TMP=$GIT_DIR/.git-save-tmp
+		GIT_INDEX_FILE=$TMP
+		export GIT_INDEX_FILE
+
+		git-read-tree $i_tree
+		git-add -u
+		w_tree=$(git-write-tree)
+		rm $TMP
+		git-read-tree --prefix=base/ HEAD^{tree}
+		git-read-tree --prefix=indx/ $i_tree
+		git-read-tree --prefix=work/ $w_tree
+		git-write-tree
+		rm $TMP
+	) )

Hmph.

I see many problems here, although the basic idea is sound and
"interesting".  I especially like the use of three trees in a
single commit, without any parenthood.

 - $GIT_DIR could contain shell metachararcters / whitespace, so
   could $TMP as well.  Always quote such variables, or you risk
   a surprise from "rm".

 - You probably would not want to create a new "save" if your
   working tree and the index are clean.  To test for the
   condition, you can do something like:

	git-diff --quiet --cached && git-diff --quiet

 - I generally prefer cleaning temporary files with "rm -f", and
   also set a trap upon exit, like this:

	# near the beginning of the script
	TMP="$GIT_DIR/.git-save-tmp-$$"
	trap 'rm -f "$TMP"' 0 
	... and then much later ...

	save=$( (
		GIT_INDEX_FILE=$TMP
		export GIT_INDEX_FILE
                ...
	) )

 - I wonder what happens if any of the commands in the above
   sequence errors out.  For example, I think you will get a
   failure from the first git-write-tree if you are in the
   middle of a merge.  You would want to error out as soon as
   you see a problem.

 - Although you keep a separate tree for the index (before the
   "git add -u" to grab the working tree changes) in the saved
   data, it does not seem to be used.  It _might_ make sense to
   replace "git add -u" with "git add ." so that work/ tree
   contains even untracked (but not ignored) files, and on the
   restore side unstage the paths that appear in work/ but not
   in indx/.  I dunno.

+
+	head=$(git-log --abbrev-commit --pretty=oneline -n 1 HEAD)
+	if branch=$(git symbolic-ref -q HEAD); then
+		branch=${branch#refs/heads/}
+	else
+		branch='(no branch)'
+	fi &&

Minor style.  Please don't write "; then\n".  Line-up "then",
"elif", "else", and "fi"; it is much easier to read that way.

+	msg=$(printf 'WIP on %s "%s"' "$branch" "$head")
+
+	saved=$(printf '%s' "$msg" | git-commit-tree "$save")
+	git update-ref -m "$msg" refs/heads/saved $saved
+	printf 'Saved %s\n' "$msg" >&2
+}
+
+function list_save () {
+	git-log --abbrev-commit --pretty=oneline -g "$@" saved
+}

I really like the way reflog is used for this.  Your saves do
not necessarily tied to a single branch (so there are no
parent-child relation between each save entry), but you can
still identify what each of the save is about because your log
message has branch name in it.

The sha1 at the beginning of oneline output, although they are
abbreviated, seem not very useful in this context, by the way.
I presume your intended use case is to view "git save list"
output, and say "git save restore saved@{2}" to apply the one
third from the top.  For that use, the commit object name is
irrelevant.  Maybe sed it out like this to make it even
prettier?

	git-log --pretty=oneline -g "$@" saved
        sed -e 's/^[0-9a-f]* //'

+function show_save () {
+	save=$(git rev-parse --verify --default saved "$1")
+	git diff-tree --stat $save:base $save:work
+}

Nice, but after trying this myself a bit, I seriously wished for
"git save show -p", so I did it myself, like this:

	show_save () {
		flags=$(git rev-parse --no-revs --flags "$@")
		if test -z "$flags"
		then
			flags=--stat
		fi
		save=$(git rev-parse --revs-only --no-flags --default saved "$@")
		git diff-tree $flags $save:base $save:work
	}

It's a dense (and ancient style) plumbing code so needs a bit of
explanation:

 - The first git-rev-parse looks at "$@", discards everything
   that are not options and discards object names.  So 'git save
   show -p some' will give you "-p" in flags.

 - The second one discards flags, and grabs 'some' out of '-p
   some', or defaults to "saved".

+function restore_save () {
+	save=$(git rev-parse --verify --default saved "$1")
+	h_tree=$(git rev-parse --verify $save:base)
+	i_tree=$(git rev-parse --verify $save:indx)
+	w_tree=$(git rev-parse --verify $save:work)
+
+	git-merge-recursive $h_tree -- HEAD^{tree} $w_tree
+}

The same "robustness" comments for the save_work function apply
here.  You probably do not want to restore on a dirty tree; the
intended use case is "stash away, pull, then restore", so I
think it is Ok to assume that you will only be restoring on a
clean state (and it would make the implementation simpler).

The three-way merge is done correctly here, and I would imagine
the users would feel the UI quite natural _if_ this merge
conflicts.  "git diff" would show only the conflicted paths
(because the updates coming from the old working tree is placed
in the index for cleanly merged paths), editing a conflicted
file and "git add $that_path" would resolve.  That's exactly the
same workflow for a conflicted merge.

However, I think it is a bit counterintuitive to update the
working tree change to the index if the merge did not conflict.
It might be better to run an equivalent of "git reset", so that
after "git save restore", the user can say "git diff" (not "git
diff HEAD") to view the local changes.  Perhaps...

	if git-merge-recursive ...
	then
        	# Cleanly merged..
		added="$TMP" &&
                git-diff --cached --name-only --diff-filter=A >"$added" &&
		git-read-tree HEAD &&
                git-update-index --add --stdin <"$added" ||
			die "Cannot unstage local modification"
		git-update-index --refresh
	fi

The --diff-filter dance is so that we do not "forget" newly
added files when we do "read-tree HEAD".

+
+if [ "$1" = list ]; then
+	shift
+	if [ "$#" = 0 ]; then
+		set x -n 10
+		shift
+	fi
+	list_save "$@"
+elif [ "$1" = show ]; then
+	shift
+	show_save "$@"
+elif [ "$1" = restore ]; then
+	shift
+	restore_save "$@"
+else
+	save_work
+	git reset --hard

I am not absolutely sure if "git reset --hard" belongs here.
You can certainly type one less command in your example sequence
("stash; pull; restore").  But I suspect there may be a case
that would be more useful if "git save" did not do the reset
itself.  I dunno....

+fi
+
+

Style.  This is a perfect place to use "case/esac".

	case "$1" in
	list)
        	do this thing
                ;;
	...
	*)
        	do default thing
                ;;
        esac

Much easier to read, isn't it?

Junio C Hamano [off-list ref] writes:

Nanako Shiraishi [off-list ref] writes:

quoted

+function save_work () {

No noiseword "function" for portability.

When you say "#!/bin/sh", you are writing for the family of
generic Bourne shells, not specifically for korn nor bash.  For
example, dash is a fine POSIX shell, but does not grok function
noiseword.  When in doubt, please stay away from things not in
POSIX (e.g. function, [[, ]], ${parameter//pattern/string/}).

Is there a good reference you can point me?

 - $GIT_DIR could contain shell metachararcters / whitespace, so
   could $TMP as well.  Always quote such variables, or you risk
   a surprise from "rm".

 - You probably would not want to create a new "save" if your
   working tree and the index are clean.  To test for the
   condition, you can do something like:

	git-diff --quiet --cached && git-diff --quiet

I did not think of these problems, but I understand now.

 - Although you keep a separate tree for the index (before the
   "git add -u" to grab the working tree changes) in the saved
   data, it does not seem to be used.  It _might_ make sense to
   replace "git add -u" with "git add ." so that work/ tree
   contains even untracked (but not ignored) files, and on the
   restore side unstage the paths that appear in work/ but not
   in indx/.  I dunno.

At first I wanted to do git-add . instead of git-add -u, but then I
became worried that will add files that are not interesting such as
temporary files.

quoted

+
+	head=$(git-log --abbrev-commit --pretty=oneline -n 1 HEAD)
+	if branch=$(git symbolic-ref -q HEAD); then
+		branch=${branch#refs/heads/}
+	else
+		branch='(no branch)'
+	fi &&

Minor style.  Please don't write "; then\n".  Line-up "then",
"elif", "else", and "fi"; it is much easier to read that way.

I see.

Nice, but after trying this myself a bit, I seriously wished for
"git save show -p", so I did it myself, like this:

	show_save () {
		flags=$(git rev-parse --no-revs --flags "$@")
		if test -z "$flags"
		then
			flags=--stat
		fi
		save=$(git rev-parse --revs-only --no-flags --default saved "$@")
		git diff-tree $flags $save:base $save:work
	}

It's a dense (and ancient style) plumbing code so needs a bit of
explanation:

 - The first git-rev-parse looks at "$@", discards everything
   that are not options and discards object names.  So 'git save
   show -p some' will give you "-p" in flags.

 - The second one discards flags, and grabs 'some' out of '-p
   some', or defaults to "saved".

I did not know about these commands.  I will study the manual pages
and I will re-submit my patch after adding these.

-- 
しらいし ななこ http://ivory.ap.teacup.com/nanako3/

----------------------------------------------------------------------
Get a free email account with anti spam protection.
http://www.bluebottle.com

Nanako Shiraishi [off-list ref] writes:

Junio C Hamano [off-list ref] writes:
...

quoted

When you say "#!/bin/sh", you are writing for the family of
generic Bourne shells, not specifically for korn nor bash.  For
example, dash is a fine POSIX shell, but does not grok function
noiseword.  When in doubt, please stay away from things not in
POSIX (e.g. function, [[, ]], ${parameter//pattern/string/}).

Is there a good reference you can point me?

I usually look at this:

    http://www.opengroup.org/onlinepubs/000095399/

and click on "Shell and Utilities volume (XCU)", and compare it
with "man bash".

At first I wanted to do git-add . instead of git-add -u, but then I
became worried that will add files that are not interesting such as
temporary files.

As long as .gitignore is set up properly to ignore such
generated files, that shouldn't be a problem.

Junio C Hamano [off-list ref] writes:

Nanako Shiraishi [off-list ref] writes:
...

quoted

+	save=$( (
+		i_tree=$(git-write-tree)
+
+		TMP=$GIT_DIR/.git-save-tmp
+		GIT_INDEX_FILE=$TMP
+		export GIT_INDEX_FILE
+
+		git-read-tree $i_tree
+		git-add -u
+		w_tree=$(git-write-tree)
+		rm $TMP
+		git-read-tree --prefix=base/ HEAD^{tree}
+		git-read-tree --prefix=indx/ $i_tree
+		git-read-tree --prefix=work/ $w_tree
+		git-write-tree
+		rm $TMP
+	) )

...
 - Although you keep a separate tree for the index (before the
   "git add -u" to grab the working tree changes) in the saved
   data, it does not seem to be used.  It _might_ make sense to
   replace "git add -u" with "git add ." so that work/ tree
   contains even untracked (but not ignored) files, and on the
   restore side unstage the paths that appear in work/ but not
   in indx/.  I dunno.

I would like to take the last part of the sentence back.

I do not think "git add ." makes much sense.  The only case that
"add ." vs "add -u" may make a difference is the files that are
so new to your working tree that you haven't even added them to
the index.  But they would not be in your current HEAD, and they
would not have been added in the commit you are pulling, and if
that is not the case, git-merge would complain and bail out as
usual anyway.  I replied to your other message with something
about ".gitignore", but not everybody uses "ignore" mechanism
effectively, and "git add ." indeed would end up sucking all the
irrelevent cruft to w_tree for them, only to be extracted back
(because HEAD, i_tree and whichever commit you will later be
applying the stash to are not likely to have them).  Maybe we
could argue that those people with nonoptimal "ignore" list
deserve it, but I do not think it is worth the potential added
aggravation.

By the way, I initially liked the clever use of a single commit
that records the three trees in it, but now I am beginning to
really hate it.  I am suspecting that the primary reason you did
it that way was to reduce the number of commit objects used for
this purpose.  I personally do not think it is something we
would want to optimize for.

I think it would make much more sense to represent a stash like
this:

              .------o commit to represent index state
             /        \
     ---o---o----------o
            HEAD       commit to represent worktree state

That is, "index" and "worktree" state are represented as one
commit each, both are direct child of the HEAD, with an added
twist of the latter being also a child of the former.

This has an interesting side effect that I can do this:

	$ git show stash

to view what was in HEAD, index and working tree at the same
time (you can use "git stash show" to view the changes between
HEAD and worktree as before).

Being able to view this "evil merge" (this is a deliberately
evil merge, as you typically have huge differences between the
index and worktree when you need to stash) may not be that
interesting in the real life, but I think this is a good
demonstration of the reason why it is better to use more
straightforward ancestry structure to represent the stash.  By
not using a custom tree structure that is only understood by
"git stash", the user can use existing tools that operate on
normal ancestry chains.

quoted

...
+	save_work
+	git reset --hard

I am not absolutely sure if "git reset --hard" belongs here.
You can certainly type one less command in your example sequence
("stash; pull; restore").  But I suspect there may be a case
that would be more useful if "git save" did not do the reset
itself.  I dunno....

I now think "git reset --hard" here is fine.

Quoting Junio C Hamano [off-list ref]:

I think it would make much more sense to represent a stash like
this:

              .------o commit to represent index state
             /        \
     ---o---o----------o
            HEAD       commit to represent worktree state

That is, "index" and "worktree" state are represented as one
commit each, both are direct child of the HEAD, with an added
twist of the latter being also a child of the former.

I do not know if I understand you correctly.
Do you mean that I should create a stash this way?

 i_tree=$(git-write-tree)
 i_commit=$(echo index | git-commit-tree $i_tree -p HEAD)
 w_tree=$( what I did to create w_tree in my previous patch )
 w_commit=$(echo $msg | git-commit-tree $w_tree -p HEAD -p $i_commit)

and when unstashing the stash, I should:

 git-merge-recursive $stash^^{tree} -- $stash^^{tree} $stash^{tree}

I think I can make it work, but if that is not what you meant, please let me know.

I am not absolutely sure if "git reset --hard" belongs here.

quoted

You can certainly type one less command in your example sequence
("stash; pull; restore").  But I suspect there may be a case
that would be more useful if "git save" did not do the reset
itself.  I dunno....

I now think "git reset --hard" here is fine.

I see.

I will try to update and resend my patch this weekend.

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

----------------------------------------------------------------------
Get a free email address with REAL anti-spam protection.
http://www.bluebottle.com