Re: [PATCH] Add git-save script

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Nanako Shiraishi <nanako3@bluebottle.com> writes:
> ...
>> +	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.

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