You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2014/02/03 21:02:50 UTC
[PATCH] pager support for command line client
I would like to discuss the idea of using a pager by default
for some commands.
This has been discussed before, e.g. here:
http://svn.haxx.se/dev/archive-2004-09/0132.shtml
and here: http://svn.haxx.se/users/archive-2009-08/0602.shtml
Both discussions were inconclusive. Generally people seem to
feel that this is a good idea, but how exactly it should be
configured is an open question.
Some other tools, such as git, have started to create some
additional mind share in this idea.
Below is a very simple proof of concept patch that enables a
pager for 'help', 'diff', and 'blame'. Currently it only looks
for a PAGER or SVN_PAGER environment variable. My plan is to
also add a 'pager-cmd' option for this purpose.
I don't expect to commit this in its current state.
I'd rather discuss the details beforehand.
I'd appreciate a test compile on windows :)
Thoughts?
(Sorry, no log message because I'm on a train that's about to arrive...)
Index: subversion/svn/blame-cmd.c
===================================================================
--- subversion/svn/blame-cmd.c (revision 1563984)
+++ subversion/svn/blame-cmd.c (working copy)
@@ -246,6 +246,7 @@ svn_cl__blame(apr_getopt_t *os,
svn_boolean_t end_revision_unspecified = FALSE;
svn_diff_file_options_t *diff_options = svn_diff_file_options_create(pool);
svn_boolean_t seen_nonexistent_target = FALSE;
+ apr_proc_t *pager_proc = NULL;
SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
opt_state->targets,
@@ -316,6 +317,9 @@ svn_cl__blame(apr_getopt_t *os,
"mode"));
}
+ if (!opt_state->non_interactive && !opt_state->xml)
+ SVN_ERR(svn_cl__start_pager(&pager_proc, pool, pool));
+
for (i = 0; i < targets->nelts; i++)
{
svn_error_t *err;
@@ -409,6 +413,9 @@ svn_cl__blame(apr_getopt_t *os,
if (opt_state->xml && ! opt_state->incremental)
SVN_ERR(svn_cl__xml_print_footer("blame", pool));
+ if (pager_proc)
+ SVN_ERR(svn_cl__close_pager(pager_proc, pool));
+
if (seen_nonexistent_target)
return svn_error_create(
SVN_ERR_ILLEGAL_TARGET, NULL,
Index: subversion/svn/cl.h
===================================================================
--- subversion/svn/cl.h (revision 1563984)
+++ subversion/svn/cl.h (working copy)
@@ -848,6 +848,22 @@ svn_cl__deprecated_merge_reintegrate(const char *s
svn_client_ctx_t *ctx,
apr_pool_t *pool);
+
+/* Launch an external pager program and redirect stdout to the pager.
+ *
+ * If the pager was started, return a pointer to the process handle
+ * for the pager in *PAGER_PROC. Else, set *PAGER_PROC to NULL. */
+svn_error_t *
+svn_cl__start_pager(apr_proc_t **pager_proc,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
+
+/* Close the pager with process handle PAGER_PROC.
+ * Must be called after all output has been written. */
+svn_error_t *
+svn_cl__close_pager(apr_proc_t *pager_proc,
+ apr_pool_t *scratch_pool);
+
#ifdef __cplusplus
}
#endif /* __cplusplus */
Index: subversion/svn/diff-cmd.c
===================================================================
--- subversion/svn/diff-cmd.c (revision 1563984)
+++ subversion/svn/diff-cmd.c (working copy)
@@ -188,6 +188,7 @@ svn_cl__diff(apr_getopt_t *os,
struct summarize_baton_t summarize_baton;
const svn_client_diff_summarize_func_t summarize_func =
(opt_state->xml ? summarize_xml : summarize_regular);
+ apr_proc_t *pager_proc = NULL;
if (opt_state->extensions)
options = svn_cstring_split(opt_state->extensions, " \t\n\r", TRUE, pool);
@@ -355,6 +356,9 @@ svn_cl__diff(apr_getopt_t *os,
svn_opt_push_implicit_dot_target(targets, pool);
+ if (!opt_state->non_interactive && !opt_state->diff.summarize)
+ SVN_ERR(svn_cl__start_pager(&pager_proc, pool, pool));
+
iterpool = svn_pool_create(pool);
for (i = 0; i < targets->nelts; ++i)
@@ -488,5 +492,8 @@ svn_cl__diff(apr_getopt_t *os,
svn_pool_destroy(iterpool);
+ if (pager_proc)
+ SVN_ERR(svn_cl__close_pager(pager_proc, pool));
+
return SVN_NO_ERROR;
}
Index: subversion/svn/help-cmd.c
===================================================================
--- subversion/svn/help-cmd.c (revision 1563984)
+++ subversion/svn/help-cmd.c (working copy)
@@ -45,6 +45,7 @@ svn_cl__help(apr_getopt_t *os,
{
svn_cl__opt_state_t *opt_state = NULL;
svn_stringbuf_t *version_footer = NULL;
+ apr_proc_t *pager_proc = NULL;
char help_header[] =
N_("usage: svn <subcommand> [options] [args]\n"
@@ -132,16 +133,24 @@ svn_cl__help(apr_getopt_t *os,
version_footer = svn_stringbuf_create(ra_desc_start, pool);
SVN_ERR(svn_ra_print_modules(version_footer, pool));
- return svn_opt_print_help4(os,
- "svn", /* ### erm, derive somehow? */
- opt_state ? opt_state->version : FALSE,
- opt_state ? opt_state->quiet : FALSE,
- opt_state ? opt_state->verbose : FALSE,
- version_footer->data,
- help_header, /* already gettext()'d */
- svn_cl__cmd_table,
- svn_cl__options,
- svn_cl__global_options,
- _(help_footer),
- pool);
+ if (!opt_state->non_interactive)
+ SVN_ERR(svn_cl__start_pager(&pager_proc, pool, pool));
+
+ SVN_ERR(svn_opt_print_help4(os,
+ "svn", /* ### erm, derive somehow? */
+ opt_state ? opt_state->version : FALSE,
+ opt_state ? opt_state->quiet : FALSE,
+ opt_state ? opt_state->verbose : FALSE,
+ version_footer->data,
+ help_header, /* already gettext()'d */
+ svn_cl__cmd_table,
+ svn_cl__options,
+ svn_cl__global_options,
+ _(help_footer),
+ pool));
+
+ if (pager_proc)
+ SVN_ERR(svn_cl__close_pager(pager_proc, pool));
+
+ return SVN_NO_ERROR;
}
Index: subversion/svn/util.c
===================================================================
--- subversion/svn/util.c (revision 1563984)
+++ subversion/svn/util.c (working copy)
@@ -33,6 +33,15 @@
#include <ctype.h>
#include <assert.h>
+#ifndef WIN32
+#include <unistd.h>
+#else
+#include <crtdbg.h>
+#include <io.h>
+#include <conio.h>
+#endif
+
+#include <apr.h> /* for STDOUT_FILENO */
#include <apr_env.h>
#include <apr_errno.h>
#include <apr_file_info.h>
@@ -1062,3 +1071,86 @@ svn_cl__propset_print_binary_mime_type_warning(apr
return SVN_NO_ERROR;
}
+svn_error_t *
+svn_cl__start_pager(apr_proc_t **pager_proc,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ apr_array_header_t *pager_cmd;
+ const char *pager;
+ const char **args;
+ apr_file_t *stdout_file;
+ apr_status_t apr_err;
+ int i;
+
+ *pager_proc = NULL;
+
+ pager = getenv("SVN_PAGER");
+ if (pager == NULL)
+ pager = getenv("PAGER");
+ /* TODO get pager-cmd from config */
+ if (pager == NULL)
+ return SVN_NO_ERROR;
+
+#ifdef WIN32
+ if (_isatty(STDOUT_FILENO) == 0)
+ return SVN_NO_ERROR;
+#else
+ if (isatty(STDOUT_FILENO) == 0)
+ return SVN_NO_ERROR;
+#endif
+
+ pager_cmd = svn_cstring_split(pager, " \t", TRUE, scratch_pool);
+ if (pager_cmd->nelts <= 0)
+ return SVN_NO_ERROR;
+ args = apr_pcalloc(scratch_pool,
+ (sizeof(const char *) * pager_cmd->nelts + 1));
+ for (i = 0; i < pager_cmd->nelts; i++)
+ args[i] = APR_ARRAY_IDX(pager_cmd, i, const char *);
+ args[i] = NULL;
+
+ apr_err = apr_file_open_stdout(&stdout_file, scratch_pool);
+ if (apr_err)
+ return svn_error_wrap_apr(apr_err, "Can't open stdout");
+
+ *pager_proc = apr_pcalloc(result_pool, sizeof(**pager_proc));
+ SVN_ERR(svn_io_start_cmd3(*pager_proc, NULL, args[0], args, NULL, TRUE,
+ TRUE, NULL,
+ FALSE, NULL,
+ FALSE, NULL,
+ result_pool));
+
+ apr_err = apr_file_dup2(stdout_file, (*pager_proc)->in, result_pool);
+ if (apr_err)
+ return svn_error_wrap_apr(apr_err, "Can't redirect stdout");
+
+ return SVN_NO_ERROR;
+}
+
+svn_error_t *
+svn_cl__close_pager(apr_proc_t *pager_proc,
+ apr_pool_t *scratch_pool)
+{
+ apr_file_t *stdout_file;
+ apr_status_t apr_err;
+ svn_error_t *err;
+
+ apr_err = apr_file_open_stdout(&stdout_file, scratch_pool);
+ if (apr_err)
+ return svn_error_wrap_apr(apr_err, "Can't open stdout");
+ SVN_ERR(svn_io_file_close(stdout_file, scratch_pool));
+ SVN_ERR(svn_io_file_close(pager_proc->in, scratch_pool));
+ err = svn_io_wait_for_cmd(pager_proc,
+ apr_psprintf(scratch_pool, "%ld",
+ (long)pager_proc->pid),
+ NULL, NULL, scratch_pool);
+ if (err && err->apr_err == SVN_ERR_EXTERNAL_PROGRAM)
+ {
+ svn_handle_warning2(stderr, err, "svn: ");
+ svn_error_clear(err);
+ }
+ else
+ SVN_ERR(err);
+
+ return SVN_NO_ERROR;
+}
Re: [PATCH] pager support for command line client
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Feb 04, 2014 at 01:07:56PM +0000, Philip Martin wrote:
> To get visible output with -F I need -X as well.
Thanks! -X makes it work for me, too.
Re: [PATCH] pager support for command line client
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Sperling <st...@elego.de> writes:
> And if I set PAGER="less -F" and run a command which produces output
> that does not fill the terminal completely I don't see any output at all.
> It seems the pager decides to exit immediately and we're writing to a
> dead fd instead of stdout. I'm not sure what to do here. However, piping
> svn's output to 'less -F' shows the same behaviour, so perhaps there
> is nothing we can do about this.
I get the same even without Subversion:
less -F repo/README.txt
generally produces no visible output, however in some terminal emulators
I do occasionally see the screen flicker. To get visible output with -F
I need -X as well.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: [PATCH] pager support for command line client
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Feb 04, 2014 at 09:14:52AM +0000, Philip Martin wrote:
> We want some pattern that ensures __close is always called even when an
> error is returned part way through. Either a pool cleanup or some sort
> of __with_pager(). Otherwise we have to ensure that none of the error
> pass bypass __close, e.g.
>
> PAGER=less svn diff wc file:///
>
> leaves me needing to reset my terminal.
That's interesting. It seems whether a terminal reset is needed depends
on the pager, because I don't see this issue with OpenBSD's less(1).
What I do see is that the error message raised by 'svn diff'
("Unable to open repository 'file://'") does not appear consistenly.
There seems to be a race between svn and the pager.
And if I set PAGER="less -F" and run a command which produces output
that does not fill the terminal completely I don't see any output at all.
It seems the pager decides to exit immediately and we're writing to a
dead fd instead of stdout. I'm not sure what to do here. However, piping
svn's output to 'less -F' shows the same behaviour, so perhaps there
is nothing we can do about this.
Re: [PATCH] pager support for command line client
Posted by Ben Reser <be...@reser.org>.
On 2/4/14, 8:26 AM, Stefan Sperling wrote:
> My current plan is to close the pager from a pool cleanup handler
> that is invoked when svn exits.
If that can be made to work reliably then that sounds great. Philip mentioned
that I was just tossing out another idea.
Re: [PATCH] pager support for command line client
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Feb 04, 2014 at 08:13:37AM -0800, Ben Reser wrote:
> On 2/4/14, 1:14 AM, Philip Martin wrote:
> > We want some pattern that ensures __close is always called even when an
> > error is returned part way through. Either a pool cleanup or some sort
> > of __with_pager().
>
> Maybe a new SVN_ERR macro?
>
> SVN_ERR_PAGER(pager, func())
>
> Which calls __close for you before returning the error?
My current plan is to close the pager from a pool cleanup handler
that is invoked when svn exits.
Re: [PATCH] pager support for command line client
Posted by Ben Reser <be...@reser.org>.
On 2/4/14, 1:14 AM, Philip Martin wrote:
> We want some pattern that ensures __close is always called even when an
> error is returned part way through. Either a pool cleanup or some sort
> of __with_pager().
Maybe a new SVN_ERR macro?
SVN_ERR_PAGER(pager, func())
Which calls __close for you before returning the error?
Re: [PATCH] pager support for command line client
Posted by Philip Martin <ph...@wandisco.com>.
Ben Reser <be...@reser.org> writes:
> On 2/3/14, 4:31 PM, Daniel Shahaf wrote:
>> Do you need to call __close if there is an error after __start? i.e.,
>> is there a state in which stdout will be printed to in the error path?
>
> Yes you do. Just try running svn diff against a non-working copy. You'll end
> up with a broken terminal that you have to reset to see what you're doing.
We want some pattern that ensures __close is always called even when an
error is returned part way through. Either a pool cleanup or some sort
of __with_pager(). Otherwise we have to ensure that none of the error
pass bypass __close, e.g.
PAGER=less svn diff wc file:///
leaves me needing to reset my terminal.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: [PATCH] pager support for command line client
Posted by Ben Reser <be...@reser.org>.
On 2/3/14, 4:31 PM, Daniel Shahaf wrote:
> Do you need to call __close if there is an error after __start? i.e.,
> is there a state in which stdout will be printed to in the error path?
Yes you do. Just try running svn diff against a non-working copy. You'll end
up with a broken terminal that you have to reset to see what you're doing.
> What about the case SVN_PAGER="/nonexistent"? Should it fall back to
> just printing to stdout, or bail out on account of failing to execute
> "/nonexistent"?
Print to stdout with a warning why the pager isn't working.
Re: [PATCH] pager support for command line client
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Mon, Feb 03, 2014 at 21:02:50 +0100:
> Thoughts?
>
> @@ -316,6 +317,9 @@ svn_cl__blame(apr_getopt_t *os,
> "mode"));
> }
>
> + if (!opt_state->non_interactive && !opt_state->xml)
> + SVN_ERR(svn_cl__start_pager(&pager_proc, pool, pool));
> +
> for (i = 0; i < targets->nelts; i++)
> {
> svn_error_t *err;
> @@ -409,6 +413,9 @@ svn_cl__blame(apr_getopt_t *os,
> if (opt_state->xml && ! opt_state->incremental)
> SVN_ERR(svn_cl__xml_print_footer("blame", pool));
>
> + if (pager_proc)
> + SVN_ERR(svn_cl__close_pager(pager_proc, pool));
> +
Do you need to call __close if there is an error after __start? i.e.,
is there a state in which stdout will be printed to in the error path?
What about the case SVN_PAGER="/nonexistent"? Should it fall back to
just printing to stdout, or bail out on account of failing to execute
"/nonexistent"?
Re: [PATCH] pager support for command line client
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On Tue, Feb 4, 2014 at 6:12 AM, Branko Čibej <br...@wandisco.com> wrote:
> I've changed my mind since and now firmly stand by the position that I
> never, ever suggested anything other than having a no-pager default. :)
>
> I'm also somewhat opposed to the idea of having pager configuration crud for
> each client subcommand. We should just hard-code it.
+1 to this version of Brane. If PAGER/SVN_PAGER is set, fine. If
not, then the command-line client had better not do anything clever
^Wstupid. -- justin
Re: [PATCH] pager support for command line client
Posted by Branko Čibej <br...@wandisco.com>.
On 04.02.2014 08:36, Ben Reser wrote:
> Branko convinced me that we should default to less (or more on
> Windows) if PAGER and SVN_PAGER is missing.
I've changed my mind since and now firmly stand by the position that I
never, ever suggested anything other than having a no-pager default. :)
I'm also somewhat opposed to the idea of having pager configuration crud
for each client subcommand. We should just hard-code it.
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: [PATCH] pager support for command line client
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Ben Reser wrote on Mon, Feb 03, 2014 at 23:36:15 -0800:
> A lot of these will make a lot more sense if we can reasonably default to
> something like the FRSX option set that git passes to less by default. They do
> this by setting the LESS environment variable (if not already set and the pager
> has no arguments).
The way they do it is broken except when $LESS is unset. When it is
set, the user-configured envvar is ignored (so, for me, -i[1] doesn't
get added) and the flags that git's use needs (e.g. -R) are not added.
They should be enabling the -FSRX options without overriding anything
else the user has enabled in $LESS. (Possibly that's just running 'less
-FSRX' or 'less -!F -!S -!R -!X' or something, and leaving the envvar
alone.)
Daniel
[1] Makes /-searches case-insensitive.
> In our case I'd suggest FX. I guess R doesn't do much for
> us at this point since we don't emit color codes and S seems like a bad idea
> for a default.
Re: [PATCH] pager support for command line client
Posted by Branko Čibej <br...@wandisco.com>.
On 04.02.2014 12:18, Stefan Sperling wrote:
> Branko also told me during a conversation we had at FOSDEM that he
> believes we should never use a pager unless PAGER or SVN_PAGER is set.
> Branko occasionally tends to contradict himself though :)
Please ... it's called "looking at both sides of the issue".
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: [PATCH] pager support for command line client
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Sperling wrote:
> On Tue, Feb 04, 2014 at 12:37:45PM +0000, Julian Foad wrote:
>> A pager for all commands when producing more than a screenful of
>> output -- fine.
>
> For this requirement, we need to count newlines and compare the
> result to the terminal window's height, so we can make a decision
> to launch a pager.
See below...
> I've got this working for svn_cmdline_fputs(). The only issue is
> that it also needs tweaks for other functions wrting to stdout,
> such as svn_stream_for_stdout().
>
> Another issue is that some people (e.g. Ben) want to use pagers
> for more than just paging, e.g. colorization. In that case we
> always want to use the pager right away. This could be made
> configurable... (use-pager=[auto|always])
>
>> Good behaviour, but don't implement it in Subversion, implement it in
>> the pager!
>
> For this requirement, we must always launch the pager upfront,
> so the pager can count the lines and decide whether to quit itself.
>
> I don't see a way to comply with both of these requirements of
> yours at the same time. Otherwise I agree with the points you've made,
> thanks!
I meant, it's a fine idea that the user should *see* the pager only when there's more than a screenful of output. In order to *implement* this, the pager should take care of the one-screenful decision and we should always launch it, not count the lines ourself.
- Julian
Re: [PATCH] pager support for command line client
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Feb 04, 2014 at 12:37:45PM +0000, Julian Foad wrote:
> A pager for all commands when producing more than a screenful of output -- fine.
For this requirement, we need to count newlines and compare the
result to the terminal window's height, so we can make a decision
to launch a pager.
I've got this working for svn_cmdline_fputs(). The only issue is
that it also needs tweaks for other functions wrting to stdout,
such as svn_stream_for_stdout().
Another issue is that some people (e.g. Ben) want to use pagers
for more than just paging, e.g. colorization. In that case we
always want to use the pager right away. This could be made
configurable... (use-pager=[auto|always])
> Good behaviour, but don't implement it in Subversion, implement it in the pager!
For this requirement, we must always launch the pager upfront,
so the pager can count the lines and decide whether to quit itself.
I don't see a way to comply with both of these requirements of
yours at the same time. Otherwise I agree with the points you've made,
thanks!
Re: [PATCH] pager support for command line client
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Feb 06, 2014 at 12:49:20PM +0100, Johan Corveleyn wrote:
> So how about making 'svn help foo' very concise and have 'svn help foo
> --verbose' give you the long version (with or without pager)? The
> concise version can end with a message "run 'svn help foo --verbose'
> for more".
+1, in any case. We can do this regardless of whether we support
an automatic pager or not.
Re: [PATCH] pager support for command line client
Posted by Johan Corveleyn <jc...@gmail.com>.
On Thu, Feb 6, 2014 at 12:32 PM, Julian Foad <ju...@btopenworld.com> wrote:
...
> But this all does make me wonder, what problem are you really trying to solve, and is making 'svn' pipe stdout to a pager the best way of solving it?
>
> You mentioned you'd be happy with just 'help' being paged. Well, maybe that's because 'svn help merge' is really a manual but we haven't written it as a man page; and 'svn help foo' usually ends with a screenful of options which probably isn't what the user primarily needs to see in most cases, and so on.
>
I agree. Why don't we focus on solving the 'svn help' problem. That
seems to be the issue that triggered this entire discussion. And it's
becoming a very complex discussion with many different preferences and
knobs ...
I too get often annoyed by 'svn help foo' if it contains too much
output. In 99% cases when I invoke 'svn help foo' I just want to see
the usage message (and perhaps a concise list of options), because I
don't remember the exact syntax. I don't want an entire manual.
So how about making 'svn help foo' very concise and have 'svn help foo
--verbose' give you the long version (with or without pager)? The
concise version can end with a message "run 'svn help foo --verbose'
for more".
--
Johan
Re: [PATCH] pager support for command line client
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Sperling wrote:
> On Tue, Feb 04, 2014 at 12:37:45PM +0000, Julian Foad wrote:
>> I do like the idea of making the output more user-friendly by using a
>> pager. But keep it simple. A pager by default just for help -- fine. A pager
>> for all commands when producing more than a screenful of output -- fine. A
>> static list of what commands we 'believe' will produce a lot of output
>> -- no.
>
> I've been using an automatic pager for every svn command for a day now.
> I'm starting to believe that we'll have to go with a static list.
>
> Turning on the pager for everything is painful, even if we tell it
> to quit itself automatically if the output doesn't fill the screen.
Hmm, that's a pity.
> Some problems I've been observing are:
>
> - Error messages are hidden by the pager even if the svn command
> produces no other output. This can be fixed for many cases by
> starting the pager as late as possible (e.g. before the subcommand
> really starts doing work). But not for all cases. I'm not sure what
> to do here. Perhaps we should pipe stderr to the pager as well?
>
> - Scripted svn commands (e.g. svn update) that produce more than a
> screenfull of output wait for the user to close the pager (arguably, the
> --non-interactive option should be passed by scripts, but the automatic
> pager can break existing scripts that don't pass this options).
> Of course, I made sure the pager isn't started if stdout is not a
> terminal, but many automated scripts are run in a terminal (e.g. builds).
>
> - If no output is produced the entire screen is cleared anyway (I
> believe this happens with git, too). This behaviour is probably
> pager-specific.
>
> Ultimately, I'd be much happier with a default behaviour where commands
> that *I* usually pipe into a pager would use an automatic pager during
> interactive use. I would guess that this applies to almost everyone.
There are no commands that *I* *usually* pipe into a pager. In nearly all situations I let the terminal be my pager: I can always
scroll back through the last few thousand lines, even in
Windows, as one of the first things I do is set the terminal window
scroll-back capacity to a large number.
Occasionally I pipe something to 'less' or 'vim -' as appropriate.
> The question then becomes which commands people usually pipe to a
> pager as a matter of habit. Should we try to make such a list or not?
If we did, it would of course be wrong for everybody.
> Perhaps the best compromise is to make just 'svn help' use a pager by
> default (so users realise that the auto-paging feature exists), and
> allow users to enable the automatic pager on a per-subcommand basis
> in the configuration file?
That sounds like the next best idea. Provide a commented-out list as an example.
But this all does make me wonder, what problem are you really trying to solve, and is making 'svn' pipe stdout to a pager the best way of solving it?
You mentioned you'd be happy with just 'help' being paged. Well, maybe that's because 'svn help merge' is really a manual but we haven't written it as a man page; and 'svn help foo' usually ends with a screenful of options which probably isn't what the user primarily needs to see in most cases, and so on.
- Julian
Re: [PATCH] pager support for command line client
Posted by Ben Reser <be...@reser.org>.
On 2/6/14, 10:39 AM, Branko Čibej wrote:
> Not in general; the pager intercepts stdout, but we prompt to and read
> responses directly from the terminal. Only in extreme cases do we fall back to
> prompting to stderr and reading responses from stdin. All of the above is true
> for Windows, too.
>
> When I rewrote the terminal abstraction for prompting, I tested it by
> redirecting both stdout and stderr to /dev/null; and I still got the prompts in
> the terminal.
I stand corrected, you're right it does work properly in spite of the pager. I
just tried svn blame with the automatic pager functionality enabled and using
less against a server that required authentication (and with --config-dir
/var/empty to force prompting). It worked perfectly.
I still think --non-interactive is not a replacement for --no-pager since in
the same example if I use --non-interactive my command doesn't run (errors out
on authenticatoin).
Re: [PATCH] pager support for command line client
Posted by Branko Čibej <br...@wandisco.com>.
On 06.02.2014 17:38, Ben Reser wrote:
> This does raise another point. This means we need to defer enabling
> the automatic pager until we are actually ready to display output
> because the pager will block prompting for passwords.
Not in general; the pager intercepts stdout, but we prompt to and read
responses directly from the terminal. Only in extreme cases do we fall
back to prompting to stderr and reading responses from stdin. All of the
above is true for Windows, too.
When I rewrote the terminal abstraction for prompting, I tested it by
redirecting both stdout and stderr to /dev/null; and I still got the
prompts in the terminal.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: [PATCH] pager support for command line client
Posted by Ben Reser <be...@reser.org>.
On 2/6/14, 12:26 AM, Branko Čibej wrote:
> Please, not another command-line option that's an alias for something we
> already do. --config-option:config:pager= should be enough. "env SVN_PAGER=
> PAGER=" also works.
>
> If we add --no-pager, we may as well add --no-editor, and I hope we can agree
> that doesn't make any sense.
--no-pager and --non-interactive do not mean the same thing. --non-interactive
implies --no-pager but not vice versa. I can very much NOT want a pager while
still wanting prompting for passwords. It would be a pain to have to type
--config-option config:helpers:enable-pager=no to get that functionality.
This does raise another point. This means we need to defer enabling the
automatic pager until we are actually ready to display output because the pager
will block prompting for passwords.
Re: [PATCH] pager support for command line client
Posted by Branko Čibej <br...@wandisco.com>.
On 06.02.2014 01:25, Ben Reser wrote:
> On 2/5/14, 1:55 PM, Stefan Sperling wrote:
>> Some problems I've been observing are:
>>
>> - Error messages are hidden by the pager even if the svn command
>> produces no other output. This can be fixed for many cases by
>> starting the pager as late as possible (e.g. before the subcommand
>> really starts doing work). But not for all cases. I'm not sure what
>> to do here. Perhaps we should pipe stderr to the pager as well?
> I think copying stderr to the pager as well is the right thing to do.
Agreed.
>> - Scripted svn commands (e.g. svn update) that produce more than a
>> screenfull of output wait for the user to close the pager (arguably, the
>> --non-interactive option should be passed by scripts, but the automatic
>> pager can break existing scripts that don't pass this options).
>> Of course, I made sure the pager isn't started if stdout is not a
>> terminal, but many automated scripts are run in a terminal (e.g. builds).
> Scripts have a couple options:
>
> 1) Disable it entirely via config file.
> 2) Unset the PAGER and SVN_PAGER environment variables if they exist.
> 3) Add --non-interactive (I think we should have a --no-pager option as well).
Please, not another command-line option that's an alias for something we
already do. --config-option:config:pager= should be enough. "env
SVN_PAGER= PAGER=" also works.
If we add --no-pager, we may as well add --no-editor, and I hope we can
agree that doesn't make any sense.
>> - If no output is produced the entire screen is cleared anyway (I
>> believe this happens with git, too). This behaviour is probably
>> pager-specific.
> This is resolved with less by using -FX. I think this is just a matter of
> people configuring their pagers appropriately. If they don't have a pager that
> handles this the way they want then they can get one or they can simply turn
> the whole thing off.
>
>> Ultimately, I'd be much happier with a default behaviour where commands
>> that *I* usually pipe into a pager would use an automatic pager during
>> interactive use. I would guess that this applies to almost everyone.
>> The question then becomes which commands people usually pipe to a
>> pager as a matter of habit. Should we try to make such a list or not?
> Well I gave my list. I obviously think we should use some sort of list and
> that it should be configurable per command because I don't think we'll ever all
> agree on which commands should have it. Also your preference may depend on
> your pager. If you're using a pager like less with -FX you may be much more
> inclined to have it turned on everywhere as opposed to someone using a pager
> that clears the screen and ends up being bothersome to them.
There's such a thing as having too many configuration knobs. I prefer a
static list.
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: [PATCH] pager support for command line client
Posted by Ben Reser <be...@reser.org>.
On 2/5/14, 1:55 PM, Stefan Sperling wrote:
> Some problems I've been observing are:
>
> - Error messages are hidden by the pager even if the svn command
> produces no other output. This can be fixed for many cases by
> starting the pager as late as possible (e.g. before the subcommand
> really starts doing work). But not for all cases. I'm not sure what
> to do here. Perhaps we should pipe stderr to the pager as well?
I think copying stderr to the pager as well is the right thing to do.
> - Scripted svn commands (e.g. svn update) that produce more than a
> screenfull of output wait for the user to close the pager (arguably, the
> --non-interactive option should be passed by scripts, but the automatic
> pager can break existing scripts that don't pass this options).
> Of course, I made sure the pager isn't started if stdout is not a
> terminal, but many automated scripts are run in a terminal (e.g. builds).
Scripts have a couple options:
1) Disable it entirely via config file.
2) Unset the PAGER and SVN_PAGER environment variables if they exist.
3) Add --non-interactive (I think we should have a --no-pager option as well).
> - If no output is produced the entire screen is cleared anyway (I
> believe this happens with git, too). This behaviour is probably
> pager-specific.
This is resolved with less by using -FX. I think this is just a matter of
people configuring their pagers appropriately. If they don't have a pager that
handles this the way they want then they can get one or they can simply turn
the whole thing off.
> Ultimately, I'd be much happier with a default behaviour where commands
> that *I* usually pipe into a pager would use an automatic pager during
> interactive use. I would guess that this applies to almost everyone.
> The question then becomes which commands people usually pipe to a
> pager as a matter of habit. Should we try to make such a list or not?
Well I gave my list. I obviously think we should use some sort of list and
that it should be configurable per command because I don't think we'll ever all
agree on which commands should have it. Also your preference may depend on
your pager. If you're using a pager like less with -FX you may be much more
inclined to have it turned on everywhere as opposed to someone using a pager
that clears the screen and ends up being bothersome to them.
> Perhaps the best compromise is to make just 'svn help' use a pager by
> default (so users realise that the auto-paging feature exists), and
> allow users to enable the automatic pager on a per-subcommand basis
> in the configuration file?
I think this is a poor choice. End users using the command line client
interactively are exactly the users who are likely to never change the
defaults. Users that are scripting or automating are exactly the users that
are going to be inclined to change the defaults.
So when you make choices like this. The average user who never bothers to look
at the config options forever ends up typing out the pipe to the pager (or just
being miserable since they don't even do that). While a few advanced users are
slightly less inconvenienced.
Just doesn't seem like a good trade off to me.
Re: [PATCH] pager support for command line client
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Feb 04, 2014 at 12:37:45PM +0000, Julian Foad wrote:
> I do like the idea of making the output more user-friendly by using a pager. But keep it simple. A pager by default just for help -- fine. A pager for all commands when producing more than a screenful of output -- fine. A static list of what commands we 'believe' will produce a lot of output -- no.
I've been using an automatic pager for every svn command for a day now.
I'm starting to believe that we'll have to go with a static list.
Turning on the pager for everything is painful, even if we tell it
to quit itself automatically if the output doesn't fill the screen.
Some problems I've been observing are:
- Error messages are hidden by the pager even if the svn command
produces no other output. This can be fixed for many cases by
starting the pager as late as possible (e.g. before the subcommand
really starts doing work). But not for all cases. I'm not sure what
to do here. Perhaps we should pipe stderr to the pager as well?
- Scripted svn commands (e.g. svn update) that produce more than a
screenfull of output wait for the user to close the pager (arguably, the
--non-interactive option should be passed by scripts, but the automatic
pager can break existing scripts that don't pass this options).
Of course, I made sure the pager isn't started if stdout is not a
terminal, but many automated scripts are run in a terminal (e.g. builds).
- If no output is produced the entire screen is cleared anyway (I
believe this happens with git, too). This behaviour is probably
pager-specific.
Ultimately, I'd be much happier with a default behaviour where commands
that *I* usually pipe into a pager would use an automatic pager during
interactive use. I would guess that this applies to almost everyone.
The question then becomes which commands people usually pipe to a
pager as a matter of habit. Should we try to make such a list or not?
Perhaps the best compromise is to make just 'svn help' use a pager by
default (so users realise that the auto-paging feature exists), and
allow users to enable the automatic pager on a per-subcommand basis
in the configuration file?
Re: [PATCH] pager support for command line client
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Sperling wrote:
> Ben Reser wrote:
>> I suspect we should allow the pager to be used with all our commands,
>> though most of them should be disabled by default. I'd say the following
>> should be enabled by default:
>> blame
>> cat
>> diff
>> list
>> log
>> mergeinfo (at least with --show-revs)
>> proplist
>> status
>
> I had a similar list. [...but...]
> I would in fact be fine with having just 'help' use a pager by default.
>
> Some commands in your list could enable the pager only when acting
> recursively, e.g. 'proplist -R', because recursive operation generally
> implies more output.
>
> We could use a static list like the above, based on what cases
> we believe will produce a lot of output.
I do like the idea of making the output more user-friendly by using a pager. But keep it simple. A pager by default just for help -- fine. A pager for all commands when producing more than a screenful of output -- fine. A static list of what commands we 'believe' will produce a lot of output -- no.
The pager should not be used when stdout is not connected to a terminal, of course.
> Or we could buffer up to one screenfull of data and decide to use the
> pager based on whether the output is going to fill the entire screen.
> This is more difficult to implement [...]
Good behaviour, but don't implement it in Subversion, implement it in the pager! GNU 'less' already has this feature built in. If Windows/BSD/whatever doesn't have such a pager by default, the Subversion packager should provide one. In the extreme, I'd rather see us write one and ship it as a tool than build such functionality into 'svn' itself.
>> A lot of these will make a lot more sense if we can reasonably default to
>> something like the FRSX option set that git passes to less by default.
For those without a copy of 'man less' in front of them:
-F or --quit-if-one-screen
Causes less to automatically exit if the entire file can be displayed on the first screen.
-R or --RAW-CONTROL-CHARS
Like -r, [Causes "raw" control characters to be displayed. The default is to display control characters using the caret notation] but only ANSI "color" escape sequences are output in "raw" form. Unlike -r, the screen appearance is maintained correctly in most cases.
-S or --chop-long-lines
Causes lines longer than the screen width to be chopped rather than folded.
-X or --no-init
Disables sending the termcap initialization and deinitialization strings to the terminal. This is sometimes desirable if the deinitialization string does something unnecessary, like clearing the screen.
> less -F is causing issues for me, where I don't see any output at all if
> the output doesn't fill screen. Not sure if this is specific to OpenBSD
> or if it can also be seen on other systems.
Do you see the same problem if you pipe the output from (non-patched) 'svn' straight into the same pager? Could it be related to '-X'? Could it be related to the sequence in which we close things:
close the svn_cmdline_stream_for_stdout
close the apr_stream_from_stdout
close the pager
flush stdout just before returning from main()
See the 'flush' call in 'main()' -- wouldn't it make more sense to set up and tear down the pager there, rather than within each individual subcommand?
- Julian
Re: [PATCH] pager support for command line client
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Feb 03, 2014 at 11:36:15PM -0800, Ben Reser wrote:
> I suspect we should allow the pager to be used with all our commands, though
> most of them should be disabled by default. I'd say the following should be
> enabled by default:
> blame
> cat
> diff
> list
> log
> mergeinfo (at least with --show-revs)
> proplist
> status
I had a similar list. I think the most obvious command that needs
a pager is 'help', because whenever I run 'svn help' I see the
bottom list of global options first while I'm usually looking for
information near the top of the output.
I would in fact be fine with having just 'help' use a pager by default.
Some commands in your list could enable the pager only when acting
recursively, e.g. 'proplist -R', because recursive operation generally
implies more output.
We could use a static list like the above, based on what cases
we believe will produce a lot of output.
Or we could buffer up to one screenfull of data and decide to use the
pager based on whether the output is going to fill the entire screen.
This is more difficult to implement (probably needs hooks in
svn_cmdline_printf() etc.) but might well be worth trying.
To avoid the problem where a command takes a lot of time to even fill
the screen, we could copy data to stdout as we buffer it and rely on
the pager to clear the screen when it starts up (less even has an option
to enforce this).
> A lot of these will make a lot more sense if we can reasonably default to
> something like the FRSX option set that git passes to less by default. They do
> this by setting the LESS environment variable (if not already set and the pager
> has no arguments). In our case I'd suggest FX. I guess R doesn't do much for
> us at this point since we don't emit color codes and S seems like a bad idea
> for a default.
less -F is causing issues for me, where I don't see any output at all if
the output doesn't fill screen. Not sure if this is specific to OpenBSD
or if it can also be seen on other systems.
> Branko convinced me that we should default to less (or more on Windows) if
> PAGER and SVN_PAGER is missing.
Branko also told me during a conversation we had at FOSDEM that he
believes we should never use a pager unless PAGER or SVN_PAGER is set.
Branko occasionally tends to contradict himself though :)
> So I'd like to see the following configuration:
> [helpers]
> enable-pager=yes|no (defaults to yes)
> pager-cmd=command [args] (defaults to SVN_PAGER, or PAGER or configure time choice)
>
> [blame]
> enable-pager=default|yes|no (default uses whatever yes/no state is on the
> helpers section)
> pager-cmd=command [args] (defaults to whatever the configuration of the helpers
> section was)
>
> and so on for additional sub-commands.
I had something like this in an early draft of my patch:
"### Set pager-cmd to the command used to invoke your external pager"NL
"### of choice. The command-line client will pipe output of commands"NL
"### listed in the 'paged-subcommands' option to the pager-cmd." NL
"### The default value of this option is 'more', unless the PAGER" NL
"### environment variable is set, which overrides the default." NL
"# pager-cmd = more" NL
"### Set paged-subcommands to a comman-separated list of subcommands"NL
"### The default is 'help', so 'svn help' will use a pager." NL
"# paged-subcommands = help" NL
If we do allow per-subcommand configuration, then a list like this
is sufficient, I think. I'm not sure it's worth bothering with a
per-subcommand configurable pager (though I do know that e.g. maxb
has mentioned this as a necessary feature).
If we manage to decide to run a pager based on the amount of output,
then I don't see a need for any client config options at all, except
perhaps a pager-cmd (so admins can pre-configure it in /etc/subversion).
> If there are objections to putting this
> in ~/.subversion/config we could put it in ~/.subversion/cmdline to make it
> clear these are really for our command line client and not everything else.
Sure.
> Part of the reason for each sub command being configurable is so you can do
> things like pagers that colorize output and what not (e.g. things like grc).
Can't a colorizing pager decide how to colorize based on its input?
Most editors detect this automatically as well.
Re: [PATCH] pager support for command line client
Posted by Ben Reser <be...@reser.org>.
On 2/3/14, 12:02 PM, Stefan Sperling wrote:
> Below is a very simple proof of concept patch that enables a
> pager for 'help', 'diff', and 'blame'. Currently it only looks
> for a PAGER or SVN_PAGER environment variable. My plan is to
> also add a 'pager-cmd' option for this purpose.
I suspect we should allow the pager to be used with all our commands, though
most of them should be disabled by default. I'd say the following should be
enabled by default:
blame
cat
diff
list
log
mergeinfo (at least with --show-revs)
proplist
status
A lot of these will make a lot more sense if we can reasonably default to
something like the FRSX option set that git passes to less by default. They do
this by setting the LESS environment variable (if not already set and the pager
has no arguments). In our case I'd suggest FX. I guess R doesn't do much for
us at this point since we don't emit color codes and S seems like a bad idea
for a default.
Branko convinced me that we should default to less (or more on Windows) if
PAGER and SVN_PAGER is missing.
So I'd like to see the following configuration:
[helpers]
enable-pager=yes|no (defaults to yes)
pager-cmd=command [args] (defaults to SVN_PAGER, or PAGER or configure time choice)
[blame]
enable-pager=default|yes|no (default uses whatever yes/no state is on the
helpers section)
pager-cmd=command [args] (defaults to whatever the configuration of the helpers
section was)
and so on for additional sub-commands. If there are objections to putting this
in ~/.subversion/config we could put it in ~/.subversion/cmdline to make it
clear these are really for our command line client and not everything else.
Part of the reason for each sub command being configurable is so you can do
things like pagers that colorize output and what not (e.g. things like grc).