Re: [PATCH 3/4] {fast-export,transport-helper}: style cleanups

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

On Fri, May 17, 2013 at 11:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> *You* are telling my that; it's *your* opinion and nothing else. It's
>
> I saw a review comment that points out that the continuation lines
> do not align, and you refused to say "ah, thanks for spotting" and
> reroll [*1*], so even I do not want to do so in general, I had to
> play the role of the arbiter.

Spotting what? There no style issues in my patch.

> My take on these style issues is this:
>
>  * People made mistakes in the past while doing real work.  Big
>    news: contributors and reviewers are not perfect.

That's exactly why "follow the surrounding code" is not a good guideline.

>  * They survived to this day because we do not do tree-wide "style
>    fixes" for the sake of style fix, in order to avoid clashing with
>    real work in flight.
>
>  * Existing mistakes are not an excuse for adding new mistakes of
>    the same kind, especially when they are pointed out during the
>    review (this is not limited to "style issues").

Fortunately nobody is adding new mistakes in this patch.

> I do not think I would reject a patch with minor style bugs like
> unaligned continuation lines, if it were a patch that does real
> work.
>
> But a "style cleanups" patch that introduces new instances of style
> breakage is a different matter.

THERE IS NO STYLE BREAKAGE.

> It is clear that the original
> (picked randomly):
>
>         die ("Encountered signed tag %s; use ",
>              "--signed-tags=<mode> to handle it.",
>              sha1_to_hex(tag->object.sha1));
>
> wanted the opening double-quotes of two lines and the "sha1" at the
> beginning of the third line to align.  I see that is the local style
> a "style cleanup" change should follow.

Unless the original had a mistaken style, or there was no guideline at
all, which is the case.

> A patch that cleans up styles in preparation for a real work (like
> this one) is a rare and precious occasion for us to really clean up
> accumulated wart.  I do not want to see existing mistakes from other
> unrelated parts of the codebase that have not been cleaned up as an
> excuse to waste that rare occasion to do a good job of cleaning up.

This is a good job of cleaning up. It follows the coding style PERFECTLY.

> So that is the arbiter's decision.  Call it *my* opinion or whatever
> you like; it does not change anything.

You are rejecting a patch for not following your *ARBITRARY* coding
style, not the project's.

All you are doing is using rhetoric to say that this patch has "broken
style" doesn't follow the "project's style", and whatever, and you
conveniently ignore the fact that 535 instances of this style, and the
fact that it's not mentioned at all in Documentation/CodingStyle
proves that it does indeed follow the project's style.

Keep using rhetoric all you want, it doesn't change the facts. You are
rejecting the patch because of your own personal subjective reasons,
and not because of anything related to the project's style.

> [Footnote]
>
> *1* That would have ended this thread without wasting everybody's
> time.

It would have ended if you applied the patch that follows the coding
style to the letter.

-- 
Felipe Contreras