Re: [PATCH] config: Use parseopt.

[Felipe Contreras <felipe.contreras@gmail.com>]

On Sat, Feb 14, 2009 at 10:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> 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.

>>> 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