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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 30, 2007, 17:19 UTC
Message-ID: <7v8xa1e4wo.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0706301641000.4438@racer.site>

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

Show 5 quoted lines

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

Show 6 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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