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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 3, 2016, 16:51 UTC
Message-ID: <xmqqfuw733p2.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAP8UFD38odsz=fwE9E+0fVJXFE-4vwL1vTQT0rshyCAUYO1fcw@mail.gmail.com>

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

Show 31 quoted lines

> 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.