Proposed New Feature (SVNROOT)

[TBrowder <tb...@cox.net>]

I previously asked about a feature like CVSROOT (to be called SVNROOT).

I have attempted to add that feature and have attached proposed code changes
in the form of diffs from the latest subversion tree.

Tom Browder

P.S.  Is there any way to get 'svn diff' to ignore changes in whitespace?  I
use 'ws-trim' in my xemacs, and the pristine subversion files I modified all
have an extra space at line endings.

Re: Proposed New Feature (SVNROOT)

[Ben Reser <be...@reser.org>]

On Tue, Mar 16, 2004 at 09:22:22AM -0500, Greg Hudson wrote:
> 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.

Actually that's in C99 now.  But we're using C89 as our target so we
shouldn't use such things as you pointed out.

-- 
Ben Reser <be...@reser.org>
http://ben.reser.org

"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Proposed New Feature (SVNROOT)

[Julian Foad <ju...@btopenworld.com>]

Greg Hudson wrote:
> So, I don't really agree with this feature, but here is some advice on
> presenting patches in this community:
[...]

Tom, since I haven't noticed your name here before, I thought I ought to say that Greg is only being helpful here.  Please don't be put off by the batch of comments.  All patches are reviewed very carefully and critically.  If you would like to read the file "HACKING" in the root of the source tree, and fix all of those points, and then re-submit your patch with "[PATCH]" at the beginning of the Subject line, it will be better received.

Also, please see if there is a corresponding issue in the issue tracker (http://subversion.tigris.org/servlets/ProjectIssues) and, if so, mention its number in the log message and the Subject line.

Note that Greg's comment "I don't really agree with this feature" might turn out to be shared by a majority of Subversion developers; we will have to see what reasons are given for and against it.

- Julian


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Proposed New Feature (SVNROOT)

[Julian Foad <ju...@btopenworld.com>]

Greg Hudson wrote:
> 
> +         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.

And the length allocated was one byte too short.

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Proposed New Feature (SVNROOT)

[Greg Hudson <gh...@MIT.EDU>]

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

Re: Proposed New Feature (SVNROOT)

[Brian Denny <br...@briandenny.net>]

Nuutti,

That's a great answer to my concern.
I hope all prospective patch-submitters read your post.

:) brian


On Wed, Mar 17, 2004 at 05:49:19AM +0200, Nuutti Kotivuori wrote:
> 
> I believe there's a bit of oranges and apples here - and definitely
> the "submit a patch" is a confusing comment and your woes are rightly
> placed here. I believe HACKING might require some clarification.
> 
> The difference comes in the nature of the discussion.
> 
> When some issue is discussed in the mailing list in general terms, the
> answers that contain the message "This is *so* low on my priority list
> that I don't even want to discuss it right now." are sometimes given
> with the comment "submit a patch".
> 
> The proper response to that is "I am willing to code this feature by
> the guidelines in HACKING and iterate the patch into Subversion, and
> this is my proposed design, please give feedback on it."
> 
> And I can guarantee people will discuss it then - if nothing else,
> then putting into words their reasons why it shouldn't be done - and
> possibly voting on it if it comes to that.
> 
> So the actual message is not ever "if you come back with a patch,
> maybe I'll consider spending my time on it" - but it is "show your
> willingness to actually work this problem through its iterations, and
> I'll spend time commenting it."
> 
> So, on the actual matter - SVNROOT patch - I think the first step
> would be to post a detailed proposal on how to handle the interface
> and get that approved.
> 
> -- Naked
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Proposed New Feature (SVNROOT)

[Nuutti Kotivuori <na...@iki.fi>]

Brian Denny wrote:
> On Tue, Mar 16, 2004 at 11:51:38PM +0200, Nuutti Kotivuori wrote:
>>
>> And unless the extent of the functionality was already discussed
>> (like I assume it wasn't from the patch), I strongly would urge you
>> to discuss the actual semantics of SVNROOT on the list first, get a
>> consensus on what would be sane behaviour, and hopefully start
>> coding after you've got a green light that this indeed is a wanted
>> change.
>
> I want to make a remark about this.
>
> I have observed a number of times on this list that a user comes to
> the list with a feature request, and they get a response something
> like "I don't really think this feature is worth the effort, but
> you're welcome to submit a patch".
>
> Which, if said before the actual semantics have been worked-out on
> list, may encourage the sort of "code-first, ask questions later"
> pattern that you (Nuutti) are trying above to discourage.
>
> Furthermore, I fear a kind of subtext-misunderstanding.  "You're
> welcome to submit a patch" sounds like encouragement, but I feel
> like sometimes people say this when what they really mean is "This
> is *so* low on my priority list that I don't even want to discuss it
> right now.  If you come back with a patch, maybe I'll consider
> spending my time on it."  -- Which is a perfectly valid attitude,
> but I just hope the potential patch submitter is hearing the right
> message.
>
> I'm not necessarily saying that either of the above comments applies
> to this particular case.

I believe there's a bit of oranges and apples here - and definitely
the "submit a patch" is a confusing comment and your woes are rightly
placed here. I believe HACKING might require some clarification.

The difference comes in the nature of the discussion.

When some issue is discussed in the mailing list in general terms, the
answers that contain the message "This is *so* low on my priority list
that I don't even want to discuss it right now." are sometimes given
with the comment "submit a patch".

The proper response to that is "I am willing to code this feature by
the guidelines in HACKING and iterate the patch into Subversion, and
this is my proposed design, please give feedback on it."

And I can guarantee people will discuss it then - if nothing else,
then putting into words their reasons why it shouldn't be done - and
possibly voting on it if it comes to that.

So the actual message is not ever "if you come back with a patch,
maybe I'll consider spending my time on it" - but it is "show your
willingness to actually work this problem through its iterations, and
I'll spend time commenting it."

So, on the actual matter - SVNROOT patch - I think the first step
would be to post a detailed proposal on how to handle the interface
and get that approved.

-- Naked

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Proposed New Feature (SVNROOT)

[Brian Denny <br...@briandenny.net>]

On Tue, Mar 16, 2004 at 11:51:38PM +0200, Nuutti Kotivuori wrote:
> 
> And unless the extent of the functionality was already discussed (like
> I assume it wasn't from the patch), I strongly would urge you to
> discuss the actual semantics of SVNROOT on the list first, get a
> consensus on what would be sane behaviour, and hopefully start coding
> after you've got a green light that this indeed is a wanted change.

I want to make a remark about this.

I have observed a number of times on this list that a user comes to
the list with a feature request, and they get a response something
like "I don't really think this feature is worth the effort, but you're
welcome to submit a patch".

Which, if said before the actual semantics have been worked-out on list,
may encourage the sort of "code-first, ask questions later" pattern that
you (Nuutti) are trying above to discourage.

Furthermore, I fear a kind of subtext-misunderstanding.  "You're welcome
to submit a patch" sounds like encouragement, but I feel like sometimes
people say this when what they really mean is "This is *so* low on my
priority list that I don't even want to discuss it right now.  If you
come back with a patch, maybe I'll consider spending my time on it."
-- Which is a perfectly valid attitude, but I just hope the potential 
patch submitter is hearing the right message. 


I'm not necessarily saying that either of the above comments applies to
this particular case.


-brian
(thinking much more about psychology than about code, lately)


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Proposed New Feature (SVNROOT)

[Nuutti Kotivuori <na...@iki.fi>]

tbrowder@cox.net wrote:
> I previously asked about a feature like CVSROOT (to be called
> SVNROOT).
>
> I have attempted to add that feature and have attached proposed code
> changes in the form of diffs from the latest subversion tree.

I probably wasn't around for the earlier discussion on SVNROOT (but I
probably were for some previous incarnation of it in the past), but in
any case, it would be very good to explain the usage of the new
feature in the mail - and not just inside the patch in code comments.

And unless the extent of the functionality was already discussed (like
I assume it wasn't from the patch), I strongly would urge you to
discuss the actual semantics of SVNROOT on the list first, get a
consensus on what would be sane behaviour, and hopefully start coding
after you've got a green light that this indeed is a wanted change.

Right now, the patch changes only 'checkout' and 'import' to use
SVNROOT, and even there the use cases are a bit odd. If something like
this is implemented, it needs to be with a clean and consistent
interface.

Other comments on the patch (aside from the ones already said):

 * There should be more description on the command-line help, and
   descriptions on all changed commands.

 * As by HACKING, you should not put your name in a comment.

 * Consider using apr_psprintf instead of sprintf.

 * Consider providing test cases for the new functionality.

Please do not be put off by the commentary received if you indeed are
serious about getting SVNROOT functionality into Subversion. Instead
let's work out the issues together so everyone will benefit.

-- Naked

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org