Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
