You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@apache.org> on 2019/11/07 16:28:35 UTC
svn status tabbed output [was: Re: svn status should not show
unmodified files in changelists]
On 07.11.2019 12:13, Daniel Shahaf wrote:
> P.S. Come to think of it, a tab-separated output mode for «svn info» might be
> nice; that is, instead of this:
> .
> % svn info README
> Path: README
> Name: README
> Working Copy Root Path: /home/daniel/src/svn/t1
> URL: https://svn.apache.org/repos/asf/subversion/trunk/README
> Relative URL: ^/subversion/trunk/README
> Repository Root: https://svn.apache.org/repos/asf
> Repository UUID: 13f79535-47bb-0310-9956-ffa450edef68
> Revision: 1869138
> Node Kind: file
> Schedule: normal
> Last Changed Author: luke1410
> Last Changed Rev: 1746277
> Last Changed Date: 2016-05-31 14:07:51 +0000 (Tue, 31 May 2016)
> Text Last Updated: 2019-09-23 18:43:22 +0000 (Mon, 23 Sep 2019)
> Checksum: 0b8d04c92f856c0d66248cdccf031fb6c0f6e05e
> Changelist: foo
> .
> we'd have this:
> .
> % svn info --tsv README
> README README /home/daniel/src/svn/t1 …
>
> This would be one line per file, so it would be easier to parse in a shell
> script.
Not with a new option. Instead --show-item should accept a
comma-separated list of keywords. That's a backwards-compatible change
that I recall being mentioned/requested a time or two (but it's also an
API change). And we could add a catch-all keyword, e.g., 'all', as
shorthand for 'print all attributes as tab-separated values'.
-- Brane
Re: svn status tabbed output [was: Re: svn status should not show
unmodified files in changelists]
Posted by Julian Foad <ju...@apache.org>.
Branko Čibej wrote:
>>> We might want to allow using multiple options, like "--show-item=revision
>>> --show-item=kind" in addition to comma-separated values.
>> I don't think this is high priority.
>
> *And* it would require a major rewrite of our argument parser, whereas
> adding a comma-separated list format would not,
Major rewrite? Why?
> and would also be compatible with -r and -c.
Whereas other existing options including '--search' and '--changelist'
already work the first way (accumulating values from multiple options).
- Julian
Re: svn status tabbed output [was: Re: svn status should not show unmodified files in changelists]
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Branko Čibej wrote on Thu, 07 Nov 2019 20:01 +00:00:
> On 07.11.2019 20:25, Daniel Shahaf wrote:
> >> We might want to allow using multiple options, like "--show-item=revision
> >> --show-item=kind" in addition to comma-separated values.
> > I don't think this is high priority.
>
> *And* it would require a major rewrite of our argument parser, whereas
> adding a comma-separated list format would not, and would also be
> compatible with -r and -c.
"Major"? We could just change opt_state.show_item from «const char *»
to an array, and parse multiple --show-item arguments the same way we
parse «svn log -r 42 -r 43». I think it'd be a trivial patch, as no other
subcommand uses --show-item, but its cost/benefit ratio would be
pretty high.
Cheers,
Daniel
Re: svn status tabbed output [was: Re: svn status should not show
unmodified files in changelists]
Posted by Branko Čibej <br...@apache.org>.
On 07.11.2019 20:25, Daniel Shahaf wrote:
> Julian Foad wrote on Thu, Nov 07, 2019 at 17:53:43 +0000:
> We would then have «svn info --rfc822
> --show-item=depth» that prints "Depth: empty" (with the RFC822 header)
Nitpick: it's not really RFC822 because there are spaces in the
"headers" ...
[...]
>> We might want to allow using multiple options, like "--show-item=revision
>> --show-item=kind" in addition to comma-separated values.
> I don't think this is high priority.
*And* it would require a major rewrite of our argument parser, whereas
adding a comma-separated list format would not, and would also be
compatible with -r and -c.
-- Brane
Re: svn status tabbed output [was: Re: svn status should not show
unmodified files in changelists]
Posted by Julian Foad <ju...@apache.org>.
Daniel Shahaf wrote:
[...]> I don't see how any of these visions — leaving aside whether
they're good ideas
> or not — is a blocker to the patch I posted. You're saying we could add other
> features, and I'm sure we could, but that's not the right question to ask. The
> question to ask is whether the patch adds value, and whether it adds something
> that we won't want to support until 2.x. I don't see any concern of the sort
> in all your points. The patch as it stands is forward-compatible with all your
> ideas.
[...]
Thank you for listening to my thoughts. I am not going to block the patch.
- Julian
Re: svn status tabbed output [was: Re: svn status should not show
unmodified files in changelists]
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Thu, Nov 07, 2019 at 17:53:43 +0000:
> We have said the outputs (except XML) simply are not intended to be parsed.
We've said that? I think what we said is that the non-XML outputs _may_ be
parsed, but with care, since they may change in minor releases as functionality
gets added. (Example: when we added tree conflicts, we added a 7th column to
the output of 'svn st'.) That's why we say «%d lines» at the end of every
«svn log | grep '^r[0-9]* '» line.
> Tab-separated output seems a rather arbitrary addition to the flock, though
> it's an OK choice in isolation for this particular use.
The rationale was to offer a line-based format for use in shell scripts and
pipelines. Shell scripts are basically the reason we added --show-item in the
first place, too.
We could use NUL separators instead of tabs.
> Better consistency with other subcommands would be achieved by using
> space-separated output with field widths chosen per field, like we chose
> them for "status" and "list".
That's not possible in this case: just consider «--show-item=foo,url,relative-url,bar».
> Or item-per-line format like the original "info" output.
Firstly, I'm not this would be worth implementing; the default 'info' output
serves well enough for that.
Secondly, that would be an unusual/unnatural interface for scripts. Line-based
formats usually operate on the premise that all lines are parsed the same way.
(Example: the output of «netstat».) «svn info», with or without the RFC822
headers at the start of each line, is the exception to the rule. The
corresponding parsing idioms (for example, Python's 'fileinput' module, and
«for line in open('foo')») operate one line at a time, not N lines at a time.
> Why should selecting items not be orthogonal to the output format?
There's a distinction between the default and XML outputs on the one hand,
and TSV output on the other hand: In the latter it's harder to select just
the particular fields one cares about.
Without the ability to select items to be output, a consumer of the
tab-separated format that cares just about a particular field would have to
either hardcode magic numbers (as in «awk 'print $1, $13'») or deal with
a header line (meaning the parsing would need to be stateful — exactly the
problem we started with — and things like «xargs -n1 svn info
--show-item=foo,bar --» would print multiple header lines). On the other hand,
in the default and XML formats selecting just the fields one cares about is easy.
So, selecting output fields is more important for TSV than for RFC822 or XML.
> The XML output should also support --show-item. It's arbitrarily
> inconsistent that it presently doesn't.
See above: consumers of the XML output can easily ignore the parts they don't
care about, even without --show-item support. On the other hand, I could see
a case for having «svn info --rfc822 --show-item=foo,bar», which would generate
the default output format but print only some of the lines, for interactive use.
More generally, I see your point that selecting fields and selecting output
format should be orthogonal. That does make sense from an abstract (pure
mathematics) point of view, but more pragmatically, I don't think it's as high
priority to support «--show-item=… --xml» as to to support multiple arguments
to «--show-item=…» in TSV mode. In fact, with info-cmd.c as it stands, supporting
«--show-item=… --xml» would not _reduce_ complexity but _increase_ it, since
several different receiver functions would need to become aware of --show-item.
> These days, JSON would be a reasonable choice of output format as an
> alternative to XML. Consider if we offered it, how would we select it?
> Perhaps a --json flag? Then why not a --tsv flag for TSV format,
Yes, we could do this. We could add --json and --tsv, as well as --rfc822 that
would select the default output mode explicitly. We could generalize that to
--output-mode=(rfc822|json|xml|tsv). We could make --show-item work in
conjunction with any of json/xml/tsv/rfc822. For compatibility reasons, «svn
info» would default to --rfc822, but --show-item would imply --tsv unless one
of --json/--xml/--rfc822 was passed. We would then have «svn info --rfc822
--show-item=depth» that prints "Depth: empty" (with the RFC822 header) and no
other lines, and we'd be able to do «svn info --json --show-item=foo» to save a
couple of microseconds to the process on the other end of the pipe.
I don't see how any of these visions — leaving aside whether they're good ideas
or not — is a blocker to the patch I posted. You're saying we could add other
features, and I'm sure we could, but that's not the right question to ask. The
question to ask is whether the patch adds value, and whether it adds something
that we won't want to support until 2.x. I don't see any concern of the sort
in all your points. The patch as it stands is forward-compatible with all your
ideas.
> and expect that too to be made available to other subcommands?
Which other commands might use TSV?
> We might want to allow using multiple options, like "--show-item=revision
> --show-item=kind" in addition to comma-separated values.
I don't think this is high priority.
Daniel
Re: svn status tabbed output [was: Re: svn status should not show
unmodified files in changelists]
Posted by Julian Foad <ju...@apache.org>.
Daniel Shahaf wrote:
> Branko Čibej wrote on Thu, Nov 07, 2019 at 17:28:35 +0100:
>> Not with a new option. Instead --show-item should accept a
>> comma-separated list of keywords.
>
> You mean, like this?
It's tempting to say +1, commit it, and say we've made an improvement.
I have been missing the ability to specify multiple arguments to
--show-item, and it scratches that itch.
But I do have some reservations and further thoughts.
I feel sad about how inconsistent our output formats are. Subcommands
such as "status" and "list" are formatted mainly using fixed-width
columns, "info" and "log" mainly using separate lines, then there are
the XML variants, and now a tab-delimited output. And, in all of these
except XML, no consistent escaping of values for machine readability.
We have said the outputs (except XML) simply are not intended to be
parsed. That is fine as far as it goes, but leaves something lacking.
Tab-separated output seems a rather arbitrary addition to the flock,
though it's an OK choice in isolation for this particular use.
Better consistency with other subcommands would be achieved by using
space-separated output with field widths chosen per field, like we chose
them for "status" and "list". Or item-per-line format like the original
"info" output. Why should selecting items not be orthogonal to the
output format?
The XML output should also support --show-item. It's arbitrarily
inconsistent that it presently doesn't. Element names used in XML
should match those in show-item; compatibility is harder to achieve for
that, because we already made them different.
These days, JSON would be a reasonable choice of output format as an
alternative to XML. Consider if we offered it, how would we select it?
Perhaps a --json flag? Then why not a --tsv flag for TSV format, and
expect that too to be made available to other subcommands?
We might want to allow using multiple options, like
"--show-item=revision --show-item=kind" in addition to comma-separated
values.
- Julian
Re: svn status tabbed output [was: Re: svn status should not show unmodified files in changelists]
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Thu, 07 Nov 2019 17:15 +00:00:
> +++ subversion/tests/cmdline/info_tests.py (working copy)
> @@ -644,6 +644,10 @@ def info_item_simple(sbox):
> + svntest.actions.run_and_verify_svn(
> + ['1\tdir'], [],
> + 'info', '--show-item=revision,kind', '--no-newline',
> + sbox.ospath(''))
I've gone ahead and committed this in r1869578, but if there are any
concerns or suggestions, they're still welcome.
Cheers,
Daniel
Re: svn status tabbed output [was: Re: svn status should not show
unmodified files in changelists]
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Branko Čibej wrote on Thu, Nov 07, 2019 at 17:28:35 +0100:
> Not with a new option. Instead --show-item should accept a
> comma-separated list of keywords.
You mean, like this?
[[[
% subversion/svn/svn info wc/foo --show-item=kind,schedule,relative-url
dir add ^/foo
%
]]]
That's not a mock-up:
[[[
Index: subversion/svn/info-cmd.c
===================================================================
--- subversion/svn/info-cmd.c (revision 1869499)
+++ subversion/svn/info-cmd.c (working copy)
@@ -411,8 +411,8 @@ typedef struct print_info_baton_t
* The following fields are used by print_info_item().
*/
- /* Which item to print. */
- info_item_t print_what;
+ /* Which items to print. */
+ apr_array_header_t *print_whats;
/* Do we expect to show info for multiple targets? */
svn_boolean_t multiple_targets;
@@ -436,7 +436,7 @@ typedef struct print_info_baton_t
* temporary allocation.
*/
static svn_error_t *
-find_print_what(const char *keyword,
+find_print_what(const char *keywords_from_argv,
print_info_baton_t *receiver_baton,
apr_pool_t *scratch_pool)
{
@@ -445,7 +445,9 @@ static svn_error_t *
svn_cl__simcheck_t *kwbuf = apr_palloc(
scratch_pool, info_item_map_len * sizeof(svn_cl__simcheck_t));
apr_size_t i;
-
+ apr_array_header_t *whats;
+
+ /* Initialize KEYWORDS and KWBUF. */
for (i = 0; i < info_item_map_len; ++i)
{
keywords[i] = &kwbuf[i];
@@ -454,9 +456,14 @@ static svn_error_t *
kwbuf[i].data = &info_item_map[i];
}
- switch (svn_cl__similarity_check(keyword, keywords,
- info_item_map_len, scratch_pool))
+ whats = svn_cstring_split(keywords_from_argv, ",", TRUE, scratch_pool);
+ for (i = 0; i < whats->nelts; i++)
{
+ const char *keyword = APR_ARRAY_IDX(whats, i, const char *);
+ switch (svn_cl__similarity_check(keyword, keywords,
+ info_item_map_len, scratch_pool))
+ /* TODO: indent this block */
+ {
const info_item_map_t *kw0;
const info_item_map_t *kw1;
const info_item_map_t *kw2;
@@ -463,8 +470,9 @@ static svn_error_t *
case 0: /* Exact match. */
kw0 = keywords[0]->data;
- receiver_baton->print_what = kw0->print_what;
- return SVN_NO_ERROR;
+ APR_ARRAY_PUSH(receiver_baton->print_whats, info_item_t) =
+ kw0->print_what;
+ break;
case 1:
/* The best alternative isn't good enough */
@@ -503,6 +511,13 @@ static svn_error_t *
" did you mean '%s', '%s' or '%s'?"),
keyword, kw0->keyword.data, kw1->keyword.data, kw2->keyword.data);
}
+ }
+
+ if (whats->nelts == 0)
+ return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
+ _("--show-item requires a non-empty argument"));
+
+ return SVN_NO_ERROR;
}
/* A callback of type svn_client_info_receiver2_t.
@@ -1125,12 +1140,18 @@ print_info_item(void *baton,
receiver_baton->path_prefix, target, pool));
const char *const target_path =
(receiver_baton->multiple_targets ? actual_target_path : NULL);
+ int i;
if (receiver_baton->start_new_line)
SVN_ERR(svn_cmdline_fputs("\n", stdout, pool));
- switch (receiver_baton->print_what)
+ for (i = 0; i < receiver_baton->print_whats->nelts; i++)
{
+ if (i > 0)
+ SVN_ERR(svn_cmdline_fputs("\t", stdout, pool));
+ switch (APR_ARRAY_IDX(receiver_baton->print_whats, i, info_item_t))
+ /* TODO: indent this block */
+ {
case info_item_kind:
SVN_ERR(print_info_item_string(svn_node_kind_to_word(info->kind),
target_path, pool));
@@ -1235,6 +1256,7 @@ print_info_item(void *baton,
default:
SVN_ERR_MALFUNCTION();
}
+ }
receiver_baton->start_new_line = TRUE;
return SVN_NO_ERROR;
@@ -1300,6 +1322,7 @@ svn_cl__info(apr_getopt_t *os,
else if (opt_state->show_item)
{
receiver = print_info_item;
+ receiver_baton.print_whats = apr_array_make(pool, 1, sizeof(info_item_t));
if (opt_state->incremental)
return svn_error_create(
Index: subversion/svn/svn.c
===================================================================
--- subversion/svn/svn.c (revision 1869499)
+++ subversion/svn/svn.c (working copy)
@@ -431,6 +431,7 @@ const apr_getopt_option_t svn_cl__options[] =
" "
"current revision (recommended when tagging)")},
{"show-item", opt_show_item, 1,
+ /* TODO: update docstring */
N_("print only the item identified by ARG:\n"
" "
" 'kind' node kind of TARGET\n"
Index: subversion/tests/cmdline/info_tests.py
===================================================================
--- subversion/tests/cmdline/info_tests.py (revision 1869499)
+++ subversion/tests/cmdline/info_tests.py (working copy)
@@ -644,6 +644,10 @@ def info_item_simple(sbox):
['1'], [],
'info', '--show-item=revision', '--no-newline',
sbox.ospath(''))
+ svntest.actions.run_and_verify_svn(
+ ['1\tdir'], [],
+ 'info', '--show-item=revision,kind', '--no-newline',
+ sbox.ospath(''))
def info_item_simple_multiple(sbox):
]]]
In addition to those TODO comments there's also a function docstring that needs
updating.
> And we could add a catch-all keyword, e.g., 'all', as shorthand for 'print
> all attributes as tab-separated values'.
I suppose we could, but I don't see what a script might use --show-item=all for.
Cheers,
Daniel