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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 30, 2007, 15:41 UTC
Message-ID: <Pine.LNX.4.64.0706301641000.4438@racer.site>
In-Reply-To: <200706300539.l5U5dHLh003989@mi1.bluebottle.com>

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