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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxx>
To: Petr Baudis <pasky@xxxxxxx>
Cc: Git Mailing List <git@xxxxxxxxxxxxxxx>
Bcc: Kirill Smelkov <kirr@xxxxxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
