[PATCH (topgit) 0/2] tg-patch: fix pagination

[PATCH (topgit) 0/2] tg-patch: fix pagination

[Kirill Smelkov]

Kirill Smelkov (2):
  Implement setup_pager just like in git
  tg-patch: fix pagination

 tg-patch.sh |    3 +++
 tg.sh       |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 0 deletions(-)

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

[Kirill Smelkov]

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>
---
 tg.sh |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/tg.sh b/tg.sh
index 8c23d26..51a82af 100644
--- a/tg.sh
+++ b/tg.sh
@@ -243,6 +243,59 @@ do_help()
 	fi
 }
 
+## Pager stuff
+
+# isatty FD
+isatty()
+{
+	tty -s 0<&$1 || return 1
+	return 0
+}
+
+# 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
+	# 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
+			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="$(mktemp -t tg-pager-fifo.XXXXXX)"
+	rm "$_pager_fifo" && 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; wait" EXIT
+}
 
 ## Startup
 
-- 
1.6.1.48.ge9b8

[PATCH (topgit) 2/2] tg-patch: fix pagination

[Kirill Smelkov]

Previously, when I was invoking `tg patch` the following used to happen:

1. .topmsg content was sent directly to _terminal_
2. for each file in the patch, its diff was generated with `git diff`
   and sent to *PAGER*
3. trailing 'tg: ...' was sent to terminal again

So the problem is that while `tg patch >file` works as expected, plain
`tg patch` does not -- in pager there is only a part of the whole patch
(first file diff) and header and trailer are ommitted.

I've finally decided to fix this inconvenience, and the way it works is
like in git -- we just hook `setup_pager` function in commands which
need to be paginated.

Signed-off-by: Kirill Smelkov <kirr@landau.phys.spbu.ru>
---
 tg-patch.sh |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/tg-patch.sh b/tg-patch.sh
index a704375..dc699d2 100644
--- a/tg-patch.sh
+++ b/tg-patch.sh
@@ -24,6 +24,9 @@ done
 base_rev="$(git rev-parse --short --verify "refs/top-bases/$name" 2>/dev/null)" ||
 	die "not a TopGit-controlled branch"
 
+
+setup_pager
+
 git cat-file blob "$name:.topmsg"
 echo
 [ -n "$(git grep '^[-]--' "$name" -- ".topmsg")" ] || echo '---'
-- 
1.6.1.48.ge9b8

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

[martin f krafft]

[-- Attachment #1: Type: text/plain, Size: 1944 bytes --]

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]:
> +# 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@madduck.net

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/) --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Adeodato Simó]

* martin f krafft [Tue, 06 Jan 2009 21:32:03 +0100]:

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

mktemp -d, and create the fifo inside the directory created by mktemp.

-- 
Adeodato Simó                                     dato at net.com.org.es
Debian Developer                                  adeodato at debian.org
 
A hacker does for love what other would not do for money.

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

[Kirill Smelkov]

Martin, thanks for your review.

I'll too reply inline:

On Tue, Jan 06, 2009 at 09:32:03PM +0100, martin f krafft wrote:
> 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]:
> > +# 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.

Agree, I'll rework isatty to be just

isatty()
{
	tty -s 0<&$1
}

> Since we are not using set -e, just "tty -s 0<&$1" in the body will
> have the same effect.

We _are_ using `set -e` in tg.sh:

http://repo.or.cz/w/topgit.git?a=blob;f=tg.sh;h=8c23d26b9a62ddcc1869bb70299862c32edd4403;hb=HEAD#l254

And also I think `tty -s 0<&$1` is a bit obscure, so maybe it is still
better to put it into a function with well-known name such as isatty?

> 
> > +	# 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}"
> 
> ?

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.

For example in git-rebase.sh:

http://repo.or.cz/w/git.git?a=blob;f=git-rebase.sh;h=ebd4df3a0e821ddcfd1eabfcaac17f854e172a85;hb=HEAD#l415

And other places.


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.

I'll add additional note about why this is needed.


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

Yes. I'd drop "cat" from here completely, but since I was mimicing
pager.c I left it:

http://repo.or.cz/w/git.git?a=blob;f=pager.c;h=f19ddbc87df04f117cd5e39189c8322fd5f29d68;hb=HEAD#l55

I'm ok with dropping cat. Should we?

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

Yes I know and agree here is a race.

But netheir did I knew how to overcome this (I though we'll need mkfifo
to support creating fifos with temp names, but mkfifo lacks support for
this)

The real problem here is that in bash, it's hard to get such constructs
going without fifos at all -- just using pipes like we would do in C,
because:

    exec >file

redirects the rest of current process output to file, but

    exec |less

does _not_ redirect the rest of the current process output through pipe
to less.

Likewise

    exec > >(less)

works (process redirection), but is a bashishm, so = not an option for
us.

Fortunately Adeodato suggested to use `mktemp -d`, so this is reworked
to:

    _pager_fifo_dir="$(mktemp -t -d tg-pager-fifo.XXXXXX)"
    _pager_fifo="$_pager_fifo_dir/0"
    mkfifo -m 600 "$_pager_fifo"

+ appopriate cleanup.

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

Thanks, corrected.

> I definitely want Petr's opinion on this before I integrate it.

Ok, let's wait what Petr says. Here is improved patch:

Changes since v1:

 o isatty() simplified
 o added a note about different meaning of GIT_PAGER='' and unset
   GIT_PAGER
 o fixed race wrt to mkfifo creation
 o fixed potential whitespace problem in "$_pager_fifo" cleanup


(interdiff)

diff --git a/tg.sh b/tg.sh
index 51a82af..bf9cf5c 100644
--- a/tg.sh
+++ b/tg.sh
@@ -248,8 +248,7 @@ do_help()
 # isatty FD
 isatty()
 {
-	tty -s 0<&$1 || return 1
-	return 0
+	tty -s 0<&$1
 }
 
 # setup_pager
@@ -259,6 +258,7 @@ 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
 	'')
@@ -283,8 +283,9 @@ setup_pager()
 	# now spawn pager
 	export LESS=${LESS:-FRSX}	# as in pager.c:pager_preexec()
 
-	_pager_fifo="$(mktemp -t tg-pager-fifo.XXXXXX)"
-	rm "$_pager_fifo" && mkfifo -m 600 "$_pager_fifo"
+	_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)
@@ -294,7 +295,7 @@ setup_pager()
 	export GIT_PAGER_IN_USE=1
 
 	# atexit(close(1); wait pager)
-	trap "exec >&-; rm $_pager_fifo; wait" EXIT
+	trap "exec >&-; rm "$_pager_fifo"; rmdir "$_pager_fifo_dir"; wait" EXIT
 }
 
 ## Startup


>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>
---
 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
+		'')
+			# 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
+}
 
 ## Startup
 
-- 
1.6.1.48.ge9b8



Thanks,
Kirill

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

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]

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.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Bert Wesarg]

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.

Bert

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 977 bytes --]

On Wed, Jan 07, 2009 at 11:27:54AM +0000, Kirill Smelkov wrote:
> Martin, thanks for your review.
> 
> I'll too reply inline:
> 
> On Tue, Jan 06, 2009 at 09:32:03PM +0100, martin f krafft wrote:
> > 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]:
> > > +# 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.
> 
> Agree, I'll rework isatty to be just
> 
> isatty()
> {
> 	tty -s 0<&$1
> }

why not test -t 0 ? I'm not sure it's POSIX though.


-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

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

[Petr Baudis]

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.

-- 
				Petr "Pasky" Baudis
The average, healthy, well-adjusted adult gets up at seven-thirty
in the morning feeling just terrible. -- Jean Kerr

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

[Kirill Smelkov]

On Wed, Jan 07, 2009 at 01:24:44PM +0100, Thomas Rast wrote:
> 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.

Good eyes, thanks!

I'll rework it.


On Wed, Jan 07, 2009 at 03:24:02PM +0100, Bert Wesarg wrote:
> 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.

Good eyes -- corrected and thanks!


On Wed, Jan 07, 2009 at 04:10:00PM +0100, Petr Baudis wrote:
> 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...?

What do you mean?

It is used in the next patch as posted in original series:

http://marc.info/?l=git&m=123125495000600&w=2

For completeness, I'll include both patches in this email.

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

Already corrected to ${GIT_PAGER-${PAGER-less}}, thanks to Thomas.

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

After we settle on implementation, would it make sense to include this
setup_pager into git-sh-setup?

I propose we include this stuff into tg.sh first, so that topgit would
work correctly with previous versions of git.

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

Pierre and Petr - thanks for the info. Yes, `test -t $fd` looks better.


Here is improved patch:

Changes since v1:

 o Simplify TG_PAGER setup  (thanks to Thomas Rast)
 o Properly escape "        (thanks to Bert Wesarg)
 o Simpler isatty           (thanks to Pierre Habouzit & Petr Baudis)


(interdiff)

diff --git a/tg.sh b/tg.sh
index bf9cf5c..b64fc3a 100644
--- a/tg.sh
+++ b/tg.sh
@@ -248,7 +248,7 @@ do_help()
 # isatty FD
 isatty()
 {
-	tty -s 0<&$1
+	test -t $1
 }
 
 # setup_pager
@@ -257,25 +257,9 @@ 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
-		'')
-			# both GIT_PAGER & PAGER unset
-			TG_PAGER=''
-			;;
-		*)
-			TG_PAGER="$PAGER"
-			;;
-		esac
-		;;
-	*)
-		TG_PAGER="$GIT_PAGER"
-		;;
-	esac
+	# TG_PAGER = GIT_PAGER | PAGER | less
+	# NOTE: GIT_PAGER='' is significant
+	TG_PAGER=${GIT_PAGER-${PAGER-less}}
 
 	[ -z "$TG_PAGER"  -o  "$TG_PAGER" = "cat" ]  && return 0
 
@@ -295,7 +279,7 @@ setup_pager()
 	export GIT_PAGER_IN_USE=1
 
 	# atexit(close(1); wait pager)
-	trap "exec >&-; rm "$_pager_fifo"; rmdir "$_pager_fifo_dir"; wait" EXIT
+	trap "exec >&-; rm \"$_pager_fifo\"; rmdir \"$_pager_fifo_dir\"; wait" EXIT
 }
 
 ## Startup


From: Kirill Smelkov <kirr@landau.phys.spbu.ru>
To: Petr Baudis <pasky@suse.cz>
Cc: Git Mailing List <git@vger.kernel.org>
Bcc: Kirill Smelkov <kirr@landau.phys.spbu.ru>
Subject: 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>

---
 tg.sh |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/tg.sh b/tg.sh
index 8c23d26..b64fc3a 100644
--- a/tg.sh
+++ b/tg.sh
@@ -243,6 +243,44 @@ do_help()
 	fi
 }
 
+## Pager stuff
+
+# isatty FD
+isatty()
+{
+	test -t $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 | less
+	# NOTE: GIT_PAGER='' is significant
+	TG_PAGER=${GIT_PAGER-${PAGER-less}}
+
+	[ -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
+}
 
 ## Startup
 
-- 
tg: (8c77c34..) t/setup-pager (depends on: master)


Second patch which uses setup_pager:


>From 1b723ebf740c58bc25ac97eff0a31b07373d8d1e Mon Sep 17 00:00:00 2001
From: Kirill Smelkov <kirr@landau.phys.spbu.ru>
Date: Tue, 6 Jan 2009 18:03:21 +0300
Subject: [TopGit PATCH] tg-patch: fix pagination

Previously, when I was invoking `tg patch` the following used to happen:

1. .topmsg content was sent directly to _terminal_
2. for each file in the patch, its diff was generated with `git diff`
   and sent to *PAGER*
3. trailing 'tg: ...' was sent to terminal again

So the problem is that while `tg patch >file` works as expected, plain
`tg patch` does not -- in pager there is only a part of the whole patch
(first file diff) and header and trailer are ommitted.

I've finally decided to fix this inconvenience, and the way it works is
like in git -- we just hook `setup_pager` function in commands which
need to be paginated.

Signed-off-by: Kirill Smelkov <kirr@landau.phys.spbu.ru>
---
 tg-patch.sh |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/tg-patch.sh b/tg-patch.sh
index a704375..dc699d2 100644
--- a/tg-patch.sh
+++ b/tg-patch.sh
@@ -24,6 +24,9 @@ done
 base_rev="$(git rev-parse --short --verify "refs/top-bases/$name" 2>/dev/null)" ||
 	die "not a TopGit-controlled branch"
 
+
+setup_pager
+
 git cat-file blob "$name:.topmsg"
 echo
 [ -n "$(git grep '^[-]--' "$name" -- ".topmsg")" ] || echo '---'
-- 
1.6.1.48.ge9b8


Thanks,
Kirill

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

[martin f krafft]

[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]

also sprach Kirill Smelkov <kirr@landau.phys.spbu.ru> [2009.01.08.1100 +1300]:
> > So I suppose you could use
> > 
> >   ${GIT_PAGER-${PAGER-less}}
> > 
> > or similar.
> 
> Good eyes, thanks!
> 
> I'll rework it.

I am not 100% on this, but I think nested {}'s are a bashism.

> On Wed, Jan 07, 2009 at 03:24:02PM +0100, Bert Wesarg wrote:
> > 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.
> 
> Good eyes -- corrected and thanks!

You could also just use single quotes inside the double quotes.

-- 
martin | http://madduck.net/ | http://two.sentenc.es/
 
"he gave me his card
 he said, 'call me if they die'
 i shook his hand and said goodbye
 ran out to the street
 when a bowling ball came down the road
 and knocked me off my feet"
                                                        -- bob dylan
 
spamtraps: madduck.bogus@madduck.net

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/) --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Kirill Smelkov]

On Thu, Jan 08, 2009 at 03:06:50PM +1300, martin f krafft wrote:
> also sprach Kirill Smelkov <kirr@landau.phys.spbu.ru> [2009.01.08.1100 +1300]:
> > > So I suppose you could use
> > > 
> > >   ${GIT_PAGER-${PAGER-less}}
> > > 
> > > or similar.
> > 
> > Good eyes, thanks!
> > 
> > I'll rework it.
> 
> I am not 100% on this, but I think nested {}'s are a bashism.

It seems to be ok:

kirr@roro3:~$ dash 
$ unset GIT_PAGER
$ unset PAGER
$ echo ${GIT_PAGER-${PAGER-less}}
less
$ PAGER=more
$ echo ${GIT_PAGER-${PAGER-less}}
more
$ GIT_PAGER=''
$ echo ${GIT_PAGER-${PAGER-less}}

$ GIT_PAGER=/bin/cat
$ echo ${GIT_PAGER-${PAGER-less}}
/bin/cat


> > On Wed, Jan 07, 2009 at 03:24:02PM +0100, Bert Wesarg wrote:
> > > 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.
> > 
> > Good eyes -- corrected and thanks!
> 
> You could also just use single quotes inside the double quotes.

Thanks for the tip - I'll keep it in mind. Or is it the preferred way?


Thanks,
Kirill

Re: [PATCH (topgit) 0/2] tg-patch: fix pagination

[Kirill Smelkov]

On Tue, Jan 06, 2009 at 06:16:37PM +0300, Kirill Smelkov wrote:
> Kirill Smelkov (2):
>   Implement setup_pager just like in git
>   tg-patch: fix pagination
> 
>  tg-patch.sh |    3 +++
>  tg.sh       |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+), 0 deletions(-)

Petr, Martin,

What's the state of this patch series?

It fixes `tg patch`, and it seems we addressed all uglyness in
setup_pager, so is there any other reason not to merge this?

Thanks,
Kirill