Re: [PATCH] Fix issue #2809

[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

[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