[PATCH] Documentation: talk about pager in api-trace.txt

[PATCH] Documentation: talk about pager in api-trace.txt

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/technical/api-trace.txt | 43 +++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/Documentation/technical/api-trace.txt b/Documentation/technical/api-trace.txt
index 097a651..a10b3a9 100644
--- a/Documentation/technical/api-trace.txt
+++ b/Documentation/technical/api-trace.txt
@@ -95,3 +95,46 @@ for (;;) {
 }
 trace_performance(t, "frotz");
 ------------
+
+Bugs & Caveats
+--------------
+
+Some git commands, like `git log`, are run by default using a
+pager. In this case, stdout and stderr are redirected to the pager and
+are closed when the pager exits.
+
+If a GIT_TRACE* environment variable has been set to "1" or "2" to
+print traces on stderr, no trace output will be printed after the
+pager has exited.
+
+This can be annoying, because GIT_TRACE_PERFORMANCE by default prints
+the performance stats for the whole command at atexit() time which
+happens after the pager has exited.
+
+So the following command will print no performance stat:
+
+------------
+GIT_TRACE_PERFORMANCE=2 git log -1
+------------
+
+To overcome this problem, you can use one of the following
+work-arounds:
+
+  - redirect to another file descriptor which is redirected to stderr,
+    like this:
+
+------------
+GIT_TRACE_PERFORMANCE=3 3>&2 git log -1
+------------
+
+  - redirect to a file specified by its absolute path, like this:
+
+------------
+GIT_TRACE_PERFORMANCE=/path/to/log/file git log -1
+------------
+
+  - use "--no-pager", like this:
+
+------------
+GIT_TRACE_PERFORMANCE=2 git --no-pager log -1
+------------
-- 
2.7.1.289.gf4cc727

Re: [PATCH] Documentation: talk about pager in api-trace.txt

[Jeff King]

On Mon, Feb 29, 2016 at 03:21:20PM +0100, Christian Couder wrote:

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  Documentation/technical/api-trace.txt | 43 +++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)

I think this is fine. I'm not sure how many people would look at the
technical/api documentation in such a case, but I don't think it hurts
to document this stuff.

-Peff

Re: [PATCH] Documentation: talk about pager in api-trace.txt

[Christian Couder]

On Mon, Feb 29, 2016 at 10:31 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Feb 29, 2016 at 03:21:20PM +0100, Christian Couder wrote:
>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>>  Documentation/technical/api-trace.txt | 43 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 43 insertions(+)
>
> I think this is fine. I'm not sure how many people would look at the
> technical/api documentation in such a case, but I don't think it hurts
> to document this stuff.

Yeah.

Junio do you plan to merge this patch or would you prefer something like:

+
+Bugs & Caveats
+--------------
+
+Some git commands, like `git log`, are run by default using a
+pager. In this case, stdout and stderr are redirected to the pager and
+are closed when the pager exits.
+
+If a GIT_TRACE* environment variable has been set to "1" or "2" to
+print traces on stderr, no trace output will be printed after the
+pager has exited.
+
+This can be annoying, because GIT_TRACE_PERFORMANCE by default prints
+the performance stats for the whole command at atexit() time which
+happens after the pager has exited.
+
+So the following command will print no performance stat:
+
+------------
+GIT_TRACE_PERFORMANCE=2 git log -1
+------------
+
+To overcome this problem, you can use for example:
+
+------------
+GIT_TRACE_PERFORMANCE=2 git --no-pager log -1
+------------

?

Re: [PATCH] Documentation: talk about pager in api-trace.txt

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> Junio do you plan to merge this patch or would you prefer something like:
>
> +
> +Bugs & Caveats
> +--------------
> +
> +Some git commands, like `git log`, are run by default using a
> +pager. In this case, stdout and stderr are redirected to the pager and
> +are closed when the pager exits.
> +
> +If a GIT_TRACE* environment variable has been set to "1" or "2" to
> +print traces on stderr, no trace output will be printed after the
> +pager has exited.
> +
> +This can be annoying, because GIT_TRACE_PERFORMANCE by default prints
> +the performance stats for the whole command at atexit() time which
> +happens after the pager has exited.
> +
> +So the following command will print no performance stat:
> +
> +------------
> +GIT_TRACE_PERFORMANCE=2 git log -1
> +------------
> +
> +To overcome this problem, you can use for example:
> +
> +------------
> +GIT_TRACE_PERFORMANCE=2 git --no-pager log -1
> +------------
>
> ?

Should I take either one?  Which one do you prefer and why?

I do not have strong preference between the two.  I understand that
the differences are only in the example workarounds.  And I do not
think the common parts of the two patches are that great.

Even though I think the first two paragraphs do not tell a lie, I
think they are overly verbose, tell irrelevant details and does not
highlight the real issue.  You only want to say

	GIT_TRACE_* environment variables can be used to tell Git to
	show trace output to its standard error stream.  Git can
	often spawn a pager internally to run its subcommand and
	send its standard output and standard error to it.

The third paragraph is misleading.  "by default prints ... at
atexit() time" sounds as if you are hinting that the solution would
be to use some non-default way to tell it to print the stats at some
other time before atexit() to ensure that the output is done before
the pager exits, but that is not actually what you are going to
suggest.

The real source of the annoyance is not that trace output will not
be seen after the pager exits, but that PERFORMANCE trace does not
begin until the pager exits, which by definition means you would see
nothing.  "This can be annoying" is an understatement (because
sending PERFORMANCE output to pager always gives an annoying
result), and blames a wrong thing at the same time (because the fact
that trace output are sent to pager together with the program output
is not what makes it annoying; the real culprit is that PERFORMANCE
output comes only after pager exits).

I'd replace it with something like this, if I were writing this patch.

	Because GIT_TRACE_PERFORMANCE trace is generated only at the
	very end of the program with atexit(), which happens after
	the pager exits, it would not work well if you send its log
	to the standard error output and let Git spawn the pager at
	the same time.

That would make "So the following ... no performance stat:"
unnecessary, and the workarounds more obvious.  You can choose not
to use the pager, or you can send the trace output to a file.