Re: multiple targets for 'svnlook propget'

On Mon, Nov 19, 2012 at 5:32 AM, Paul Burba <ptburba_at_gmail.com> wrote:
> On Fri, Nov 16, 2012 at 1:04 PM, Paul Burba <ptburba_at_gmail.com> wrote:
>> On Fri, Nov 16, 2012 at 10:58 AM, Julian Foad
>> <julianfoad_at_btopenworld.com> wrote:
>>>
>>> --
>>> Subversion Live 2012 - 2 Days: training, networking, demos & more! http://bit.ly/NgUaRi
>>> Certified & Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
>>>
>>> ----- Original Message -----
>>>> From: Johan Corveleyn <jcorvel_at_gmail.com>
>>>> To: Julian Foad <julianfoad_at_btopenworld.com>
>>>> Cc: Subversion Development <dev_at_subversion.apache.org>
>>>> Sent: Friday, 16 November 2012, 8:54
>>>> Subject: Re: multiple targets for 'svnlook propget'
>>>>
>>>> On Fri, Nov 16, 2012 at 1:19 PM, Julian Foad <julianfoad_at_btopenworld.com>
>>>> wrote:
>>>>> Johan Corveleyn wrote:
>>>>>
>>>>>> Right now, 'svnlook propget' supports only a single
>>>> PATH_IN_REPOS
>>>>>> argument:
>>>>>>
>>>>>> usage: 1. svnlook propget REPOS_PATH PROPNAME PATH_IN_REPOS
>>>>>>
>>>>>> I think it would be useful to support multiple targets, like 'svn
>>>>>> propget'. That would make it much faster to gather all props set
>>>>>> during a transaction, for instance in a pre-commit hook:
>>>>>>
>>>>>> svnlook propget -t $TXN $REPOS svn:eol-style `svnlook changed -t
>>>>>> $TXN $REPOS`
>>>>>>
>>>>>> The only complication I see, is that the output for multiple targets
>>>>>> has to write the node for which the properties are shown.
>>>>>>
>>>>>> Looking at 'svn propget' for inspiration, we see that it
>>>> doesn't
>>>>>> output the node when there is only one target. But it does when there
>>>>>> are multiple targets. I guess 'svnlook propget' could implement
>>>> the
>>>>>> same behavior, which means that the output for single target remains
>>>>>> backwards compatible (at the cost of slight increase of complexity for
>>>>>> scripts that parse that output -> they have to know if they passed
>>>> one
>>>>>> or mutiple targets).
>>>>>>
>>>>>> Current output of 'svn propget':
>>>>>>
>>>>>> $ svn pg svn:eol-style ^/file.txt
>>>>>> native
>>>>>> $ svn pg svn:eol-style ^/file.txt ^/binary.exe ^/file2.txt
>>>>>> http://my.svn.server/svn/file.txt - native
>>>>>> http://my.svn.server/svn/file2.txt - native
>>>>>>
>>>>>> (As you can see, the it simply does not output the targets that
>>>> don't
>>>>>> have the property, and shows the actual target for every file that
>>>>>> does)
>>>>>>
>>>>>> Or with working copy targets:
>>>>>>
>>>>>> $ svn pg svn:eol-style file.txt binary.exe file2.txt
>>>>>> file.txt - native
>>>>>> file2.txt - native
>>>>>>
>>>>>>
>>>>>> Now, when we look at the output of 'svnlook propget':
>>>>>>
>>>>>> $ svnlook pg repos svn:eol-style file.txt
>>>>>> native
>>>>>>
>>>>>> We could have following multi-target output:
>>>>>>
>>>>>> $ svnlook pg repos svn:eol-style file.txt binary.exe file2.txt
>>>>>> file.txt - native
>>>>>> file2.txt - native
>>>>>>
>>>>>>
>>>>>> Thoughts?
>>>>>>
>>>>>>
>>>>>> P.S.: This multi-target output isn't very parseable for multi-line
>>>>>> properties. Currently, 'svn propget' also has this problem, but
>>>> users
>>>>>> can use --xml to get machine-readable output.
>>>>>
>>>>> I added a "svn propget --verbose" mode (in 1.7 I think) to
>>>> resolve that problem. It uses the same format as "svn proplist
>>>> --verbose", which is like this:
>>>>>
>>>>> [[[
>>>>> $ svn pg -vR svn:ignore # selected bits of output are shown
>>>>> Properties on 'subversion/bindings/ctypes-python/csvn/ext':
>>>>> svn:ignore
>>>>> *.pyc
>>>>> Properties on 'subversion/mod_dav_svn/reports':
>>>>> svn:ignore
>>>>> .libs
>>>>>
>>>>> Properties on 'contrib/server-side/svnstsw/include':
>>>>> svn:ignore
>>>>> Makefile
>>>>> Makefile.in
>>>>>
>>>>> ]]]
>>>>>
>>>>> If the email system hasn't mangled the white space, you should be able
>>>> to see that the prop name is indented by two spaces and the value by four
>>>> spaces, and newlines in the value are preserved (with exactly one LF added to
>>>> ensure there is a line break in the output) so it's fully parseable.
>>>>>
>>>>>> Maybe 'svnlook propget'
>>>>>> should eventually also grow a --xml option to handle this. But I'd
>>>> say
>>>>>> that's yet another feature. For now, I would be happy if svnlook
>>>> could
>>>>>> already do "plain" output for multiple targets, which can
>>>> already be
>>>>>> useful/parseable for single-line properties like svn:eol-style.
>>>>>
>>>>> Given that 'svnlook' is very much expected to be used by scripts
>>>> ('svn' serves most human-interaction cases), I don't see
>>>> implementing the "plain" output is a good idea.
>>>>>
>>>>> But +1 on implementing the "svn pg -v" format.
>>>>
>>>> Ok, but then you probably mean that I should add two things:
>>>> 1) Add multi-target support to 'svnlook pg'
>>>> 2) Add -v to 'svnlook pg', format like 'svn pg -v'
>>>>
>>>> The combination of 1 and 2 would then give us a parseable output for
>>>> multiple targets.
>>>>
>>>> But I guess I shouldn't forbid the use of 1 without 2, right? I think
>>>> it would be strange to say "cannot use multiple arguments without
>>>> --verbose" ...
>>>> So it seems I should also provide a format for the multi-arg case
>>>> without -v ... like the current format of 'svn pg' without -v.
>>>>
>>>>
>>>> I also note that the same enhancement can't easily be made for
>>>> proplist. Mainly because 'svnlook pl' already has a --verbose option,
>>>> so we can't change the output of 'svnlook pl --verbose' with a
>>>> single
>>>> target without breaking backwards compatibility.
>>>>
>>>> - 'svn pl' always shows "Properties on 'XXX':"
>>>> (whether for single
>>>> target or multiple targets, whether verbose or not)
>>>> - 'svnlook pl' does not, and when used with --verbose it doesn't
>>>> indent multiline properties like 'svn pl' does.
>>>>
>>>> [[[
>>>> $ svn pl sources --verbose
>>>> Properties on 'sources':
>>>> svn:ignore
>>>> .classpath
>>>> .settings
>>>> .project
>>>>
>>>> $ svnlook pl repos trunk/sources --verbose
>>>> svn:ignore : .classpath
>>>> .settings
>>>> .project
>>>> ]]]
>>>>
>>>> Currently I'm mainly focused on propget, so proplist doesn't bother me
>>>> too much. But it would of course be even better if proplist behavior
>>>> would match the propget behavior. If anyone has any ideas for this ...
>>>
>>> It seems to me that the 'svnlook pl --verbose' output is pretty useless. Well, OK, not useless, but poor. IIRC 'svn pl --verbose' was like that and I changed it to use the new format, at the same time as I changed 'svn pg --verbose' to use the new format. In terms of back-compat, however, 'svn pl --verbose' didn't exist before I added it, whereas I changed the output of 'svn pl --verbose' in an incompatible way. I simply decided that the latter was an acceptable change.
>>>
>>> Apart from backward compatibility concerns, I would totally go for making 'svn pg' and 'svn pl' and 'svnlook pg' and 'svnlook pl' all use the indented & parseable format, and not relate it to the '--verbose' option at all.
>>
>> +1
>>
>>> I dunno, add a '--parseable' (or should that be '--parsable') option to select the new format?
>>
>> I prefer the backward incompatible format change over a new option.
>> It's a slippery slope if, every time we want to change the format of a
>> command line program, we add a new option. What if we later change
>> the format of the optioned output?
>
> Johan,
>
> Are you planning on making any of these changes? I'm curious because
> obviously it would impact any work I do to add --show-inherited-props
> support to svnlook [propget|proplist]

Hi Paul,

I'd like to do this, yes, but I have to admit the time I can spend on
it is unpredictable (in between work, kids, and construction works on
our house), so I'm not really sure when I'll get around to it. And I
certainly don't want to hold you or anyone back on making other
changes.

Another option is, if you (or anyone) have some more time to spare, go
ahead and make these changes yourself if you'd like. If you're in the
code there anyway, for the inherited-props stuff, maybe it's easy to
tackle these other things as well?

Apart from looking at the code, I haven't done any work on this yet.

(on a related note, I currently have a patch sitting here for adding
--diff-cmd to 'svnlook diff', but those changes are local to the
print_diff_tree function).

Thanks,

-- 
Johan