[Quilt-dev] Re: [PATCH] pager support like git

[Andreas Gruenbacher]

Bert,

I like this feature. There are a few little remaining problems though.

> diff --git a/quilt/diff.in b/quilt/diff.in
> index 8435024..866d61c 100644
> --- a/quilt/diff.in
> +++ b/quilt/diff.in
> @@ -196,7 +196,7 @@ do
>                       opt_color=1 ;;
>               auto | tty)
>                       opt_color=
> -                     [ -t 1 ] && opt_color=1 ;;
> +                     [ -t 1 -o -n "${QUILT_PAGER_IN_USE-}" ] && opt_color=1 
> ;;
>               never)
>                       opt_color= ;;
>               *)
> @@ -310,6 +310,8 @@ then
>       || die 1
>  fi
>  
> +setup_pager
> +
>  for file in "address@hidden"
>  do
>       if [ -n "$opt_snapshot" -a -e "$QUILT_PC/$snap_subdir/$file" ]

Here and in some other places, setup_pager is called after evaluating 
QUILT_PAGER_IN_USE though. This looks wrong.

> diff --git a/quilt/scripts/patchfns.in b/quilt/scripts/patchfns.in
> index 9723685..ad0b10b 100644
> --- a/quilt/scripts/patchfns.in
> +++ b/quilt/scripts/patchfns.in
> @@ -968,6 +968,41 @@ quilt_command()
>       QUILT_COMMAND="" bash $BASH_OPTS -c "${SUBDIR:+cd $SUBDIR;} . 
$QUILT_DIR/$command" "quilt $command" "$@"
>  }
>  
> +# 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
> +
> +     # QUILT_PAGER = GIT_PAGER | PAGER | less
> +     # NOTE: GIT_PAGER='' is significant
> +     QUILT_PAGER=${GIT_PAGER-${PAGER-less}}
> +
> +     [ -z "$QUILT_PAGER"  -o  "$QUILT_PAGER" = "cat" ]  && return 0
> +
> +
> +     # now spawn pager
> +     export LESS="${LESS:-FRSX}"     # as in pager.c:pager_preexec()

Hmm, what is FRSX here?

> +
> +     _pager_fifo_dir="$(mktemp -t -d quilt-pager-fifo.XXXXXX)"
> +     _pager_fifo="$_pager_fifo_dir/0"
> +     mkfifo -m 600 "$_pager_fifo"
> +
> +     "$QUILT_PAGER" < "$_pager_fifo" &
> +     exec > "$_pager_fifo"           # dup2(pager_fifo.in, 1)
> +
> +     export QUILT_PAGER_IN_USE=1
> +
> +     # atexit(close(1); wait pager)
> +     trap "exec >&-; rm \"$_pager_fifo\"; rmdir \"$_pager_fifo_dir\"; wait" 
EXIT
> +}
> +
>  #
>  # If the working directory does not contain a $QUILT_PATCHES directory,
>  # quilt searches for its base directory up the directory tree. If no

Also, quilt already uses trap in some places, so we have a conflict here.

Do we really need a named pipe here, or would an unnamed pipe do? Example:

        #! /bin/bash
        exec > >(sed -e 's/^/FILTERED: /')
        echo foo


Thanks,
Andreas