Thread (2 messages) flat view 2 messages, 2 authors, 2016-06-15

Re: [PATCH] config: Use parseopt.

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:10

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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help