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

> Kirill Smelkov wrote:
> > On Tue, Jan 06, 2009 at 09:32:03PM +0100, martin f krafft wrote:
> > > I find this very confusing. Why not simply
> > > 
> > >   TG_PAGER="${GIT_PAGER:-}"
> > >   TG_PAGER="${TG_PAGER:-$PAGER}"
> > > 
> > > ?
> > 
> > I find it confusing too, but this is needed because they usually do
> > something like this
> > 
> >     $ GIT_PAGER='' <some-git-command>
> > 
> > to force it to be pagerless.
> [...]
> > So I think it would be better to preserve the same semantics for `tg
> > patch` callers, though it's a pity that it's hard (maybe I'm wrong ?) to
> > see whether an env var is unset.
> 
> Admittedly I haven't really studied your patch, but the ${} constructs
> can in fact tell empty from unset:
> 
>   $ EMPTY=
>   $ unset UNDEFINED
>   $ echo ${UNDEFINED-foo}
>   foo
>   $ echo ${UNDEFINED:-foo}
>   foo
>   $ echo ${EMPTY-foo}
> 
>   $ echo ${EMPTY:-foo}
>   foo
> 
> 'man bash' indeed says
> 
>   When not performing substring expansion, bash tests for a parameter
>   that is unset or null; omitting the colon results in a test only for
>   a parameter that is unset.
> 
> So I suppose you could use
> 
>   ${GIT_PAGER-${PAGER-less}}
> 
> or similar.

> On Wed, Jan 7, 2009 at 12:27, Kirill Smelkov <kirr@landau.phys.spbu.ru> wrote:
> > Martin, thanks for your review.
> > +       # atexit(close(1); wait pager)
> > +       trap "exec >&-; rm "$_pager_fifo"; rmdir "$_pager_fifo_dir"; wait" EXIT
> I think you need to escape the double quotes.

> On Wed, Jan 07, 2009 at 02:27:54PM +0300, Kirill Smelkov wrote:
> > >From 2193b7c703c2d31c8739eec617b8c0e8c1d09b79 Mon Sep 17 00:00:00 2001
> > From: Kirill Smelkov <kirr@landau.phys.spbu.ru>
> > Date: Tue, 6 Jan 2009 17:56:37 +0300
> > Subject: [PATCH (topgit) v2] Implement setup_pager just like in git
> > 
> > setup_pager() spawns a pager process and redirect the rest of our output
> > to it.
> > 
> > This will be needed to fix `tg patch` output in the next commit.
> > 
> > Signed-off-by: Kirill Smelkov <kirr@landau.phys.spbu.ru>
> 
> But you never use it...?

> > ---
> >  tg.sh |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 54 insertions(+), 0 deletions(-)
> > 
> > diff --git a/tg.sh b/tg.sh
> > index 8c23d26..bf9cf5c 100644
> > --- a/tg.sh
> > +++ b/tg.sh
> > @@ -243,6 +243,60 @@ do_help()
> >  	fi
> >  }
> >  
> > +## Pager stuff
> > +
> > +# isatty FD
> > +isatty()
> > +{
> > +	tty -s 0<&$1
> > +}
> > +
> > +# setup_pager
> > +# Spawn pager process and redirect the rest of our output to it
> > +setup_pager()
> > +{
> > +	isatty 1 || return 0
> > +
> > +	# TG_PAGER = GIT_PAGER | PAGER
> > +	# (but differentiate between GIT_PAGER='' and unset variables)
> > +	# http://unix.derkeiler.com/Newsgroups/comp.unix.shell/2004-03/0792.html
> > +	case ${GIT_PAGER+XXX} in
> > +	'')
> > +		case ${PAGER+XXX} in
> > +		'')
> 
> I'm pretty sure there's been a nice trick for this, but I can't remember
> it at all now.

> > +			# both GIT_PAGER & PAGER unset
> > +			TG_PAGER=''
> > +			;;
> > +		*)
> > +			TG_PAGER="$PAGER"
> > +			;;
> > +		esac
> > +		;;
> > +	*)
> > +		TG_PAGER="$GIT_PAGER"
> > +		;;
> > +	esac
> > +
> > +	[ -z "$TG_PAGER"  -o  "$TG_PAGER" = "cat" ]  && return 0
> > +
> > +
> > +	# now spawn pager
> > +	export LESS=${LESS:-FRSX}	# as in pager.c:pager_preexec()
> > +
> > +	_pager_fifo_dir="$(mktemp -t -d tg-pager-fifo.XXXXXX)"
> > +	_pager_fifo="$_pager_fifo_dir/0"
> > +	mkfifo -m 600 "$_pager_fifo"
> > +
> > +	"$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"; rmdir "$_pager_fifo_dir"; wait" EXIT
> > +}
> 
> Frankly, I would have been just much happier if something like git
> pager--helper would be provided for external tools to use. Seeing how it
> gets reimplemented like this just pains me greatly.

> On Wed, Jan 07, 2009 at 03:44:32PM +0100, Pierre Habouzit wrote:
> > On Wed, Jan 07, 2009 at 11:27:54AM +0000, Kirill Smelkov wrote:
> > > isatty()
> > > {
> > > 	tty -s 0<&$1
> > > }
> > 
> > why not test -t 0 ? I'm not sure it's POSIX though.
> 
> It's SUS for many issues already it seems.