Re: [PATCH (topgit) 1/2] Implement setup_pager just like in git

From: martin f krafft <madduck@madduck.net>
Date: Jan 6, 2009, 20:32 UTC
Message-ID: <20090106203203.GA11274@lapse.rw.madduck.net>
In-Reply-To: <acaae74f79d385014e726b97f8258b2a0caa3dd0.1231254832.git.kirr@landau.phys.spbu.ru>

Thanks, Kirill, for the patches. A couple of comments inline. I hope Petr has a chance to look too.

also sprach Kirill Smelkov <kirr@landau.phys.spbu.ru> [2009.01.06.1616 +0100]:

Show 6 quoted lines

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

Show 7 quoted lines

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

Show 9 quoted lines

> +	"$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@madduck.net