> -----Original Message-----
> From: Mark Phippard [mailto:MarkP@softlanding.com]
> Posted At: Sunday, July 24, 2005 9:37 PM
> Posted To: gmane.comp.version-control.subversion.devel
> Conversation: [PATCH] Issue 1628
> Subject: Re: [PATCH] Issue 1628
>
> Branko Èibej <brane@xbc.nu> wrote on 07/24/2005 08:27:00 PM:
>
> > I think you're wrong about svnserve not restarting -- it can and
will,
> > if it's wrapped with some kind of service wrapper.
> >
> > But more important: we're talking about server administration here.
> > Servers usually run unattended. The biggest pain in the ass about
> > Windows (as a server OS) is that 99% of the server software running
on
> > it assumes that someone will always monitor the server through a
GUI.
> > Your crash report code even assumes that the box actually has a mail
> > client installed.
> >
> > An administrator can view the event log remotely (without a GUI
login),
> > and can get at files on the server remotely.
> >
> >
> > The patch itself is almsot there, I only counted one tab, a few
cases of
>
> > hungarian notation, and some funny indentaion and space-before-paren
not
>
> > consistent with the rest of the file.
> >
> > But before we go and iron out these nits, let's solve the problem of
the
>
> > format of the crash reports themselves, please.
>
> I have to say, if I were Stefan I would be a bit frustrated right now.
> This is how I see what has happened here.
>
> Stefan submits a code example based on how he has integrated this
feature
> into TortoiseSVN. He makes is pretty clear that he is not submitting
a
> patch to be applied as is, but rather a suggestion as to a possible
way to
> solve this issue.
>
> His submission gets picked apart repeatedly over style issues with
> virtually no mention of the technical details and merits of the
approach.
> Only after he resubmits it several times correcting said style issues
is
> it now brought up that this approach has some significant areas that
would
> need to be addressed.
>
> I have no problem with the merit of the disagreement over the details
of
> this approach but why couldn't you or anyone else have engaged Stefan
on
> these issues based on his original submission, or at least the one
where
> he corrected the log message? I understand, agree with and even
admire
> the attention that goes into patch review on this list, but isn't
there
> any room for a non-committer to submit code and generate a discussion?
I
> see this as a real missed opportunity. This project could use another
> person or two that is paying close attention to how this software runs
on
> Windows. This submission could have been an opportunity to attract a
new
> committer. The technical problems with this approach could have been
> raised up front, possibly with some encouragement attached to the
response
> to see if Stefan wanted to try to solve those problems. If he decided
to
> solve those problems and came back with a new patch, then you could
have
> raised the issue about supplying a patch that met the coding standards
of
> the project. Or perhaps he would have taken the time to do so on his
own,
> since now he would know he was submitting an actual patch. Instead, I
> would just be surprised if Stefan is going to feel any motivation to
take
> this any further and see if the crashrpt.dll could be modified to dump
the
> information to a file instead of requiring email and a GUI.
>
> End of rant. This wasn't really directed at just you brane, I am
seeing
> this more and more as the project is growing and people are probably
> feeling more overwhelmed as there are so many proposals flying around.
It
> just seems like it ought to be possible to detect when something comes
> across the list that is more of an idea, then a patch. That idea
could be
> discussed a bit, and if it gets somewhere and the person that
submitted
> the idea wants to go back and work on a patch, then they could perhaps
be
> reminded of the coding guidelines so that they are prepared for those
> comments when they eventually submit the patch.
>
> Just some thoughts.
>
> Thanks
>
> Mark
>
I want to add in my observations to this as well. I've been reading the
list for a few days now and I'm just astounded at the process it takes
to get a patch committed. As an outsider looking in, your methods are
very draconian. I can't recall the number of patch threads I've read in
the last couple days but I walk away from every one of them with the
same thought in mind: These guys critique more on "ugly" code than they
do on poorly written code.
The overwhelming majority of the reviewer critiques center around code
formatting. There is a very minor amount of actual critiques over code
problems, just formatting. IMO, if it's so important that your code be
formatted so perfectly, use a tidy program. I find it ridiculous that
you _force_ people to use your coding conventions to submit a patch.
These people do all the work of writing and testing the code and you
throw it right back in their face with style issues.
I understand that there is some merit to code consistency and all that,
however, your methods are to the extreme. I will __never__ submit a
patch to Subversion simply because I will not put up with your
haranguing on style. If I go to all the trouble of writing some code
that _you don't have to_, so long as it works correctly and is
appropriate to include (based on the code itself and not its style),
then I fully expect you to "get over it" and accept it. If you have
formatting issues, well, that's why there are tidy programs. I consider
it a waste of time for both the patch submitter and the reviewer(s) to
constantly reject a patch for styling issues. The patch write has done
the bulk of the work for you, which is to write and test the code. The
very least you could do is fire up an editor or tidy program and fix up
their code formatting if its that important to you.
Again, I want to emphasize this. I've consumed a large amount of this
list in a few days so I've seen a lot of threads all at once. In
virtually every patch thread, the reviewers have found more formatting
errors than anything. And there were more than a few comments from the
committers who committed a patch begrudgingly because there were still a
few minor formatting errors. I consider committing a patch but still
making a side comment about "there were style errors but I fixed them"
to be begrudging.
Speaking specifically about Stefan, he made it quite clear he was giving
you a proof of concept patch very early on. I'm surprised he's put as
much effort into this as he has given his personality which I've seen in
action on the TortoiseSVN list. He reminds me a bit of me, which is to
say, often times not very patient with people. Your first priority with
the proof of concept code was to get it into your preferred style. Once
that is done, and only then, did you actually comment on the merits of
the code itself.
Did I wonder into the twilight zone? In my realm, the merits of the
code take precedence over... well, anything, really. I can write you
some beautiful code... that doesn't work. Would that be more likely to
speed up the patch committing process? At the moment, I'm only half
joking when I say that I think I could just about get away with writing
perfectly formatted code (Per the subversion manifesto) that has
absolutely nothing to do with the log message describing the
feature/problem.
I'm not directing my comments at anybody in particular (Well not
exactly, my comments are directed at "cat COMMITTERS", I suppose). The
style guidelines stated in the HACKING file should be just that,
guidelines; recommendations. They should not be mandatory to the point
of having patches rejected over minutia. I wonder how many potential
developers have been turned away for the same reasons I am or have
cracked the HACKING file and seen that it's enormous. I'm sorry, but
I'm not reading an 1800 line guide just to write code in your stead.
I've a great many better things to do than read 1800+ lines to even
stand a chance of getting a patch accepted within the first 4 versions
due to style issues. Those better things including watching paint dry,
counting the ticks of a clock, counting the individual pixels on my
monitor and stapling my crotch.
Some of my comments are said somewhat tongue-in-cheek. But before
anybody goes and gets their panties in a bunch, stop and think for a
minute and review some of the recent patch threads over the last 3 or 4
weeks. Stand outside the Subversion world for a minute and think, what
if you wanted to submit to a project and they wanted their code to use
Hungarian Notation (Which I gather is apparently the devil's mark in
source code around here). Take yourself out of this little bubble and
look in from the outside. I think you guys are smart enough to see that
you've gotten your priorities more than a little misplaced with regards
to reviewing and accepting patches. I just hope that somewhere along
the line you stop and take a long hard look at what these people are
doing; which is to say, things that you are not doing so you should be
just a bit more grateful for less stylish code and more willing to fix
up styling problems since that's a trivial exercise compared to the task
of writing and testing code.
- Valik
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jul 25 04:32:49 2005