Re: Proposed New Feature (SVNROOT)

So, I don't really agree with this feature, but here is some advice on
presenting patches in this community:

  * To post a message to this list, don't respond to an existing message
and change the subject, as convenient as that may be. That leaves
behind references headers which (in some people's mail-readers) will
mis-thread your message.

  * Don't tar up the patch and log message and send it as an
attachment. Either send one text attachment with the log message and
one with the patch, or send a single text attachment with both.
(Inlining the patch as plain text is also okay if your MUA won't mangle
it.)

  * Your log message does not conform to the standards in HACKING, even
a little bit. (It's just a two-line description of the change; no
description of how it modifies symbols in the source code.)

  * Give more information in the message body about what your patch does
to the command-line syntax, so we don't have to read the source patch to
figure out what you're trying to do.

  * By hook or by crook, do not send patches which include unrelated
whitespace changes. I suggest turning off ws-trim in xemacs when
working on other people's code, but "svn diff --diff-cmd=diff -x -uw"
might also work for you.

  * We don't really make much use of #if 0'd debugging code, so don't
include that in your patches.

I didn't review the patch carefully, but this jumped out at me:

+ char buf[strlen(svnroot)+strlen(local_dir)+1];
+ sprintf(buf, "%s/%s", svnroot, local_dir);

You've made use of a gcc extension here; in regular old C, arrays must
have constant length. Use apr_psprintf() instead.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Mar 16 15:22:53 2004