[PATCH] request-pull: do not paginate output of git commands

[PATCH] request-pull: do not paginate output of git commands

[Michal Marek]

git request-pull called inside a terminal prints part of the output to
the terminal and other parts are piped through the pager. Fix this.

Signed-off-by: Michal Marek <mmarek@suse.cz>
---
 git-request-pull.sh |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-request-pull.sh b/git-request-pull.sh
index a2cf5b8..b3aaded 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -34,7 +34,7 @@ branch=$(git ls-remote "$url" \
 	}")
 if [ -z "$branch" ]; then
 	echo "warn: No branch of $url is at:" >&2
-	git log --max-count=1 --pretty='format:warn:   %h: %s' $headrev >&2
+	git --no-pager log --max-count=1 --pretty='tformat:warn:   %h: %s' $headrev >&2
 	echo "warn: Are you sure you pushed $head there?" >&2
 	echo >&2
 	echo >&2
@@ -45,13 +45,13 @@ fi
 PAGER=
 export PAGER
 echo "The following changes since commit $baserev:"
-git shortlog --max-count=1 $baserev | sed -e 's/^\(.\)/  \1/'
+git --no-pager shortlog --max-count=1 $baserev | sed -e 's/^\(.\)/  \1/'
 
 echo "are available in the git repository at:"
 echo
 echo "  $url $branch"
 echo
 
-git shortlog ^$baserev $headrev
-git diff -M --stat --summary $merge_base $headrev
+git --no-pager shortlog ^$baserev $headrev
+git --no-pager diff -M --stat --summary $merge_base $headrev
 exit $status
-- 
1.6.3

Re: [PATCH] request-pull: do not paginate output of git commands

[Junio C Hamano]

Michal Marek <mmarek@suse.cz> writes:

> git request-pull called inside a terminal prints part of the output to
> the terminal and other parts are piped through the pager. Fix this.

Hmph, I have always thought this was meant to be a feature.

That is, you run it to yourself, instead of piping it to a file or "| mail
torvalds", in order to sanity check before you actually do the latter.
When the request is larger than a screenful, you would want a pager while
reviewing.

Admittedly, I do not regularly use request-pull, so I wouldn't know if use
of the pager is inconvenient to people who have to use this feature
regularly. Your patch could be addressing a real usability issue.

But I have this nagging suspicion that you may be lacking F (or FX) from
your LESS environment variable?

Re: [PATCH] request-pull: do not paginate output of git commands

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Michal Marek <mmarek@suse.cz> writes:
>
>> git request-pull called inside a terminal prints part of the output to
>> the terminal and other parts are piped through the pager. Fix this.
>
> Hmph, I have always thought this was meant to be a feature.
>
> That is, you run it to yourself, instead of piping it to a file or "| mail
> torvalds", in order to sanity check before you actually do the latter.
> When the request is larger than a screenful, you would want a pager while
> reviewing.

Sorry, I take it back.

If it _were_ a single call to a paging command what I said may make sense,
but with many separate calls to shortlog, we wouldn't know which output is
too large (and uses pager).  I agree your patch makes things better.

I should have read your commit log message after finishing my coffee ;-)

Thanks.

Re: [PATCH] request-pull: do not paginate output of git commands

[Michal Marek]

Junio C Hamano napsal(a):
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Michal Marek <mmarek@suse.cz> writes:
>>
>>> git request-pull called inside a terminal prints part of the output to
>>> the terminal and other parts are piped through the pager. Fix this.
>> Hmph, I have always thought this was meant to be a feature.
>>
>> That is, you run it to yourself, instead of piping it to a file or "| mail
>> torvalds", in order to sanity check before you actually do the latter.
>> When the request is larger than a screenful, you would want a pager while
>> reviewing.
> 
> Sorry, I take it back.
> 
> If it _were_ a single call to a paging command what I said may make sense,
> but with many separate calls to shortlog, we wouldn't know which output is
> too large (and uses pager).  I agree your patch makes things better.

Yes, this is what I dislike, it prints parts to the terminal and parts
are paged (separately). Paging the whole output would be a nice
improvement, but probably not worth the effort.


> I should have read your commit log message after finishing my coffee ;-)

:-)

Michal

Re: [PATCH] request-pull: do not paginate output of git commands

[Junio C Hamano]

Michal Marek <mmarek@suse.cz> writes:

> git request-pull called inside a terminal prints part of the output to
> the terminal and other parts are piped through the pager. Fix this.
>
> Signed-off-by: Michal Marek <mmarek@suse.cz>
> ---

Having said all that, I noticed this gem in the context in your patch.

>  git-request-pull.sh |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/git-request-pull.sh b/git-request-pull.sh
> index a2cf5b8..b3aaded 100755
> --- a/git-request-pull.sh
> +++ b/git-request-pull.sh
> @@ -45,13 +45,13 @@ fi
>  PAGER=
>  export PAGER
>  echo "The following changes since commit $baserev:"

Looking at ff06c74 (Improve request-pull to handle non-rebased branches,
2007-05-01), it introduced these specifically to address the issue you are
bringing up, but it is ineffective.

I'd suggest to replace your patch with the attached instead.

---

 git-request-pull.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-request-pull.sh b/git-request-pull.sh
index a2cf5b8..f86d8fd 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -42,8 +42,8 @@ if [ -z "$branch" ]; then
 	status=1
 fi
 
-PAGER=
-export PAGER
+GIT_PAGER=cat
+export GIT_PAGER
 echo "The following changes since commit $baserev:"
 git shortlog --max-count=1 $baserev | sed -e 's/^\(.\)/  \1/'
 

Re: [PATCH] request-pull: do not paginate output of git commands

[Michal Marek]

On Tue, Jun 30, 2009 at 11:28:26AM -0700, Junio C Hamano wrote:
> Looking at ff06c74 (Improve request-pull to handle non-rebased branches,
> 2007-05-01), it introduced these specifically to address the issue you are
> bringing up, but it is ineffective.
> 
> I'd suggest to replace your patch with the attached instead.
> 
> ---
> 
>  git-request-pull.sh |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/git-request-pull.sh b/git-request-pull.sh
> index a2cf5b8..f86d8fd 100755
> --- a/git-request-pull.sh
> +++ b/git-request-pull.sh
> @@ -42,8 +42,8 @@ if [ -z "$branch" ]; then
>  	status=1
>  fi
>  
> -PAGER=
> -export PAGER
> +GIT_PAGER=cat
> +export GIT_PAGER

Good idea, but for completeness, it should be set before this command:
    git log --max-count=1 --pretty='format:warn:   %h: %s' $headrev >&2
(which should use tformat to get the newline right). So what about this
one?

---

diff --git a/git-request-pull.sh b/git-request-pull.sh
index ab2dd10..5917773 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -12,6 +12,9 @@ OPTIONS_SPEC=
 . git-sh-setup
 . git-parse-remote
 
+GIT_PAGER=
+export GIT_PAGER
+
 base=$1
 url=$2
 head=${3-HEAD}
@@ -34,7 +37,7 @@ branch=$(git ls-remote "$url" \
 	}")
 if [ -z "$branch" ]; then
 	echo "warn: No branch of $url is at:" >&2
-	git log --max-count=1 --pretty='format:warn:   %h: %s' $headrev >&2
+	git log --max-count=1 --pretty='tformat:warn:   %h: %s' $headrev >&2
 	echo "warn: Are you sure you pushed $head there?" >&2
 	echo >&2
 	echo >&2
@@ -42,8 +45,6 @@ if [ -z "$branch" ]; then
 	status=1
 fi
 
-GIT_PAGER=
-export GIT_PAGER
 echo "The following changes since commit $baserev:"
 git shortlog --max-count=1 $baserev | sed -e 's/^\(.\)/  \1/'
 

Re: [PATCH] request-pull: do not paginate output of git commands

[Junio C Hamano]

Michal Marek <mmarek@suse.cz> writes:

> Good idea, but for completeness, it should be set before this command:
>     git log --max-count=1 --pretty='format:warn:   %h: %s' $headrev >&2
> (which should use tformat to get the newline right). So what about this
> one?

Good eyes.

The blamed commit for that new use of "log" is ff06c74 (Improve
request-pull to handle non-rebased branches, 2007-05-01) itself, which did
introduce the "define an empty PAGER to disable paging" trick, so this
makes it doubly buggy (i.e. for forgetting GIT_PAGER trumps PAGER, and for
introducing a use of log that is not covered by its own trick).

Thanks.