git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Kirill Smelkov <kirr@landau.phys.spbu.ru>
Date
Jan 7, 2009, 11:27 UTC
Message-ID
<20090107112754.GA15158@roro3>
In-Reply-To
<20090106203203.GA11274@lapse.rw.madduck.net>
Martin, thanks for your review.
I'll too reply inline:
On Tue, Jan 06, 2009 at 09:32:03PM +0100, martin f krafft wrote:
Show 13 quoted lines
> 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?

Show 15 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}"
> 
> ?

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.
Show 5 quoted lines
> 
> > +     [ -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?
Show 5 quoted lines
> > +	_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.
Show 13 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.
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
Previous: Adeodato SimóNext: Thomas Rast
Message 5 of 14 in “tg-patch: fix pagination”
  1. 0/2 tg-patch: fix paginationKirill Smelkov, Jan 6, 2009
  2. 1/2 Implement setup_pager just like in gitKirill Smelkov, Jan 6, 2009
  3. martin f krafftJan 6, 2009
  4. Adeodato SimóJan 7, 2009
  5. Kirill SmelkovJan 7, 2009
  6. Thomas RastJan 7, 2009
  7. Bert WesargJan 7, 2009
  8. Pierre HabouzitJan 7, 2009
  9. Petr BaudisJan 7, 2009
  10. Kirill SmelkovJan 7, 2009
  11. martin f krafftJan 8, 2009
  12. Kirill SmelkovJan 8, 2009
  13. 2/2 tg-patch: fix paginationKirill Smelkov, Jan 6, 2009
  14. Kirill SmelkovJan 18, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.