You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Senthil Kumaran S <se...@collab.net> on 2008/06/26 13:01:50 UTC
[PATCH] Fix issue #2809
Hi,
I am attaching a patch along with this email, which fixes issue #2809, ie.,
adding support '--xml' support for 'svnlook plist'.
This patch passes all 'make *check'.
[[[
Fix issue #2809: svnlook proplist output should be machine parsable (e.g. XML)
* subversion/svnlook/main.c
(): Include svn_xml.h, svn_base64.h.
(enum): Add svnlook__xml_opt.
(options_table, cmd_table, svnlook_opt_state): Add new option for --xml.
(print_xml_prop): New function to print props in xml which is same as
svn_cl__print_xml_prop.
(do_plist): Accept opt_state for xml and throw output in xml if opted.
(subcommand_plist): Call do_plist with xml option state.
(main): Add a case to see whether user provided --xml.
Patch by: stylesen
]]]
Thank You.
--
Senthil Kumaran S
http://www.stylesen.org/
Re: [PATCH] Fix issue #2809
Posted by Senthil Kumaran S <se...@collab.net>.
Hi Julian,
Julian Foad wrote:
> I tweaked just a little bit more. I noticed that your "print_xml_prop()"
> function is an exact copy of "svn_cl__print_xml_prop()". I added a
> comment above it, saying so, so that when someone is modifying it later
> they will realise that they may have to make their modifications in two
> places to maintain consistency. When you copy and paste a significant
> amount of code, it's good to point this out, so that the reviewer knows
> it was a deliberate decision, because we normally try to avoid doing
> this.
I made a note of this in the log message. In future will do it inside the code
as you have pointed out, since that seems right for maintaining it.
> I've committed this in 31978 and marked the issue as fixed.
Thank you for the commit.
--
Senthil Kumaran S
http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2809
Posted by Julian Foad <ju...@btopenworld.com>.
On Mon, 2008-06-30 at 14:52 +0530, Senthil Kumaran S wrote:
> Updated the log message.
Thanks.
[Re. the difference in XML elements between "svn pl --revprop" and
"svnlook pl --revprop".]
> > Is there a reason for this difference?
>
> Actually, I missed it during my initial development of this patch. Thanks for
> pointing it out.
That's looking good now.
> > There's one warning when I build with this patch:
> >
> > subversion/svnlook/main.c:1548: warning: no previous prototype for
> > 'print_xml_prop'
>
> The updated patch must not give this warning.
It doesn't. That's good.
I tweaked just a little bit more. I noticed that your "print_xml_prop()"
function is an exact copy of "svn_cl__print_xml_prop()". I added a
comment above it, saying so, so that when someone is modifying it later
they will realise that they may have to make their modifications in two
places to maintain consistency. When you copy and paste a significant
amount of code, it's good to point this out, so that the reviewer knows
it was a deliberate decision, because we normally try to avoid doing
this.
I think it's OK to leave this one as a copy for now, as it's not
necessary that the two versions remain identical and the amount of code
is not large, but we should keep an eye on the situation and (especially
if we start duplicating more code) we should consider sharing it like we
share svn_cmdline__getopt_init().
I've committed this in 31978 and marked the issue as fixed.
Thanks for the patch!
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2809
Posted by Senthil Kumaran S <se...@collab.net>.
Hi Julian,
Julian Foad wrote:
>>From reading the patch and the rest of the log message, I see that this
> patch leave the regular output just how it was, and adds an "--xml"
> option for giving XML output when the user wants it, which is the
> solution suggested in the issue tracker. It would be good to say this in
> the log message summary paragraph so that the reader does not have to go
> and investigate.
Updated the log message.
> Is there a reason for this difference?
Actually, I missed it during my initial development of this patch. Thanks for
pointing it out.
> There's one warning when I build with this patch:
>
> subversion/svnlook/main.c:1548: warning: no previous prototype for
> 'print_xml_prop'
The updated patch must not give this warning.
I ve attached an updated patch with the above changes and log message changed.
Julian, Thanks for your review.
--
Senthil Kumaran S
http://www.stylesen.org/
Re: [PATCH] Fix issue #2809
Posted by Julian Foad <ju...@btopenworld.com>.
On Thu, 2008-06-26 at 18:31 +0530, Senthil Kumaran S wrote:
> I am attaching a patch along with this email, which fixes issue #2809, ie.,
> adding support '--xml' support for 'svnlook plist'.
>
> This patch passes all 'make *check'.
>
> [[[
> Fix issue #2809: svnlook proplist output should be machine parsable (e.g. XML)
Hi Senthil. That's great.
From reading the patch and the rest of the log message, I see that this
patch leave the regular output just how it was, and adds an "--xml"
option for giving XML output when the user wants it, which is the
solution suggested in the issue tracker. It would be good to say this in
the log message summary paragraph so that the reader does not have to go
and investigate.
For a rev-prop, "svnlook" outputs an element called <target> without an
indication of the revision number, where "svn" outputs an element like
<revprops rev="1">:
> $ bin/svnlook pl --xml --revprop -r 1 obj-dir/svn-test-work/repositories/prop_tests-1/
> <?xml version="1.0"?>
> <properties>
> <target>
> <property
> name="svn:log">Log message for revision 1.</property>
> <property
> name="svn:author">jrandom</property>
> <property
> name="svn:date">2008-05-29T17:57:40.166685Z</property>
> </target>
> </properties>
> $ bin/svn pl --xml --revprop -r 1 obj-dir/svn-test-work/working_copies/prop_tests-1/
> <?xml version="1.0"?>
> <properties>
> <revprops
> rev="1">
> <property
> name="svn:log"/>
> <property
> name="svn:author"/>
> <property
> name="svn:date"/>
> </revprops>
> </properties>
Is there a reason for this difference?
There's one warning when I build with this patch:
subversion/svnlook/main.c:1548: warning: no previous prototype for
'print_xml_prop'
This is an observation for a separate improvement that is outside the
scope of your patch: "svn plist --revprop" requires that a revision
number is specified; "svnlook" should do the same.
Thanks.
- Julian
> * subversion/svnlook/main.c
> (): Include svn_xml.h, svn_base64.h.
> (enum): Add svnlook__xml_opt.
> (options_table, cmd_table, svnlook_opt_state): Add new option for --xml.
> (print_xml_prop): New function to print props in xml which is same as
> svn_cl__print_xml_prop.
> (do_plist): Accept opt_state for xml and throw output in xml if opted.
> (subcommand_plist): Call do_plist with xml option state.
> (main): Add a case to see whether user provided --xml.
>
> Patch by: stylesen
> ]]]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org