Felipe Contreras [off-list ref] writes:
quoted
I personally do not think "I rewrote this command's option parser using
parseopt" earns any "trust point". I think the latter is a *great* thing
to do, though.
I disagree. Making a patch pass through all the filters must mean
something, and the more patches the more trust.
Why are you arguing?
I am saying I do not feel more trust in people _merely because_ they
rewrote command parser to use parseopt. Telling me that I am wrong and I
should trust you more for such a patch would not help.
quoted
Mistakes made in the past and resulting flaws that remain in the current
source do not justify a new patch adding more mistakes to the codebase.
Reviewers help the author from adding more.
One bad thing about the current process (and I am certainly one of the
guilty parties because I do a lot of reviews) around this area is that a
review comment that points out a mistake similar to the ones made in the
past sound to the author of the patch as if the reviewer is telling that
the patch will not be accepted unless the existing mistakes are also fixed
by the patch author. Such a review certainly does not mean that ...
...
But, if there's code that already has the same issues the patch has,
it doesn't look like a good argument for patch rejection. Perhaps the
quality standards increased, but on the other hand things wouldn't get
worst by applying the patch.
If you read the above quoted block again, you will notice that we are
almost in full agreement, *if* you change "by applying the patch" in your
last sentence to "by applying a patch that is revised to fix the problem
pointed out during the review in it, even if it does not address the
similar ones in the existing code".
Adding more breakages of the same kind may not increase the number of
classes of breakages, but surely it increases the number of places that
need to be fixed later, and it is actively wrong to discard time and
energy somebody already spent to prevent one more instance of known
breakage to get into the codebase.
On Sat, Feb 14, 2009 at 10:35 PM, Junio C Hamano [off-list ref] wrote:
Felipe Contreras [off-list ref] writes:
quoted
quoted
I personally do not think "I rewrote this command's option parser using
parseopt" earns any "trust point". I think the latter is a *great* thing
to do, though.
I disagree. Making a patch pass through all the filters must mean
something, and the more patches the more trust.
Why are you arguing?
I am saying I do not feel more trust in people _merely because_ they
rewrote command parser to use parseopt. Telling me that I am wrong and I
should trust you more for such a patch would not help.
I'm not arguing, that's just my opinion.
I'm not saying writing a patch should give anyone penguin points, it's
going through the review process up to patch acceptance. Maybe not to
you, but maybe for other developers.
quoted
quoted
Mistakes made in the past and resulting flaws that remain in the current
source do not justify a new patch adding more mistakes to the codebase.
Reviewers help the author from adding more.
One bad thing about the current process (and I am certainly one of the
guilty parties because I do a lot of reviews) around this area is that a
review comment that points out a mistake similar to the ones made in the
past sound to the author of the patch as if the reviewer is telling that
the patch will not be accepted unless the existing mistakes are also fixed
by the patch author. Such a review certainly does not mean that ...
...
But, if there's code that already has the same issues the patch has,
it doesn't look like a good argument for patch rejection. Perhaps the
quality standards increased, but on the other hand things wouldn't get
worst by applying the patch.
If you read the above quoted block again, you will notice that we are
almost in full agreement, *if* you change "by applying the patch" in your
last sentence to "by applying a patch that is revised to fix the problem
pointed out during the review in it, even if it does not address the
similar ones in the existing code".
Adding more breakages of the same kind may not increase the number of
classes of breakages, but surely it increases the number of places that
need to be fixed later, and it is actively wrong to discard time and
energy somebody already spent to prevent one more instance of known
breakage to get into the codebase.
I think are in full agreement, it's just that I wasn't clear enough;
those are not the kind of issues I was talking about. If there's a
known way how to do something then it's obvious the patch should be
re-submitted with the 'right way'. I was thinking more on "FILE" vs
"file", or any kind of issue that would require a separate patch to
reach consistency in the existing code; those can wait until after the
original patch is accepted.
--
Felipe Contreras