Re: [PATCH v5 0/4] commit: Add commit.verbose configuration

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 16, 2014, 20:28 UTC
Message-ID: <xmqqsin4ppjw.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140614041452.GA1375@hudson.localdomain>

Jeremiah Mahler <jmmahler@gmail.com> writes:

Show 13 quoted lines

> On Fri, Jun 13, 2014 at 11:49:10AM -0500, Caleb Thompson wrote:
> ...
>> > The patches look good, they apply clean ('git am'), and all tests pass.
>> >
>> > Reviewed-by: Jeremiah Mahler <jmmahler@gmail.com>
>> 
>> So that I'm clear on the etiquitte, is it appropriate for me to add this
>> Reviewed-by line to the commit messages at this point, provided that the
>> patches don't change?
>
> I am not sure of the etiquette either.  But personally, since I hardly
> contributed anything, I don't think it is necessary to put my tag in
> your patches.

It is correct that it is not necessary for Caleb to resend these patches if it is done only to add "Reviewed-by:" from you (but it does not hurt to do so, either).

In the review process, "Reviewed-by:" sent for a patch by one of the trusted reviewers will reduce workload from other reviewers because there will be one less reason to read such a patch [*1*]. A corollary to this is that other reviewers will ignore an extra "Reviewed-by:" by somebody whose review quality is still unknown, so it will not hurt.

Your reading others patches and commenting on is appreciated very much, and sending a "Reviewed-by:" is perfectly fine. As you gain experience and as others see your review comments more and more, people will start trusting your reviews. Everybody begins at a "novice" state, after all ;-)

By the way, speaking of netiquette, please refrain from using "Mail-Followup-To:" while working on this list [*2*].

Thanks.

[Footnotes]

*1* We usually read a patch for one of the two reasons: either the reviewer personally finds what the patch wants to do interesting and worthwhile, and wants to make sure it is done in the right way. Or the reviewer thinks that applying the patch is detrimental to the overall project, perhaps the design and/or the implementation is wrong, and point the problems out. A "Reviewed-by" from a trusted reviewer will allow other reviewers to simply skip/ignore the patch if what it does is "Meh" to them, saying "I am not particularly interested, but as long as it does not hurt, I would not be opposed, and the other guy reviewed and says it would not hurt, and I tend to trust his judgment." The maintainer does not have the luxury of skip/ignore such a patch because he needs to at least apply and test the integration result, though ;-).

Now, who are the "trusted reviewers"? Anybody who is knows s/he is one of them ;-)

*2* http://thread.gmane.org/gmane.comp.version-control.git/165477/focus=165549