Thanks, Kirill, for the patches. A couple of comments inline. I hope
Petr has a chance to look too.
also sprach Kirill Smelkov <kirr@xxxxxxxxxxxxxxxxxxx> [2009.01.06.1616 +0100]:
> +# isatty FD
> +isatty()
> +{
> + tty -s 0<&$1 || return 1
> + return 0
> +}
You don't need any of the return statements. Functions' return
values are the return values of the last commands they execute.
Since we are not using set -e, just "tty -s 0<&$1" in the body will
have the same effect.
> + # TG_PAGER = GIT_PAGER | PAGER
> + # http://unix.derkeiler.com/Newsgroups/comp.unix.shell/2004-03/0792.html
> + case ${GIT_PAGER+XXX} in
> + '')
> + case ${PAGER+XXX} in
> + '')
> + # both GIT_PAGER & PAGER unset
I find this very confusing. Why not simply
TG_PAGER="${GIT_PAGER:-}"
TG_PAGER="${TG_PAGER:-$PAGER}"
?
> + [ -z "$TG_PAGER" -o "$TG_PAGER" = "cat" ] && return 0
What if I set my pager to /bin/cat? But I suppose then there's just
a wasted FIFO and process, nothing big.
> + _pager_fifo="$(mktemp -t tg-pager-fifo.XXXXXX)"
> + rm "$_pager_fifo" && mkfifo -m 600 "$_pager_fifo"
There's a race condition here. I can't see a real problem with it,
though, nor do I know of a better way.
> + "$TG_PAGER" < "$_pager_fifo" &
> + exec > "$_pager_fifo" # dup2(pager_fifo.in, 1)
> +
> + # this is needed so e.g. `git diff` will still colorize it's output if
> + # requested in ~/.gitconfig with color.diff=auto
> + export GIT_PAGER_IN_USE=1
> +
> + # atexit(close(1); wait pager)
> + trap "exec >&-; rm $_pager_fifo; wait" EXIT
Consistency: $_pager_fifo is not passed as a quoted string to rm
here. In the unlikely event that $TMPDIR contains a space, this
would fail.
I definitely want Petr's opinion on this before I integrate it.
--
martin | http://madduck.net/ | http://two.sentenc.es/
if you see an onion ring -- answer it!
spamtraps: madduck.bogus@xxxxxxxxxxx
Attachment:
digital_signature_gpg.asc
Description: Digital signature (see http://martin-krafft.net/gpg/)
