Re: [PATCH v5 3/3] upload-pack: optionally allow fetching reachable sha1

From: Junio C Hamano <gitster@pobox.com>
Date: May 23, 2015, 01:01 UTC
Message-ID: <xmqq617kds4r.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CABA5-zm3LqA+BAHj0EegoXmTuaNobhEHyD0v2kg=VfCzzqx-+A@mail.gmail.com>

Fredrik Medley <fredrik.medley@gmail.com> writes:

Show 9 quoted lines

> 2015-05-22 0:15 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
>
>> It looks like this new set of tests are well thought out; good job.
>>
>> I spotted a few minor nits, though.  All I'll amend while applying
>> so there is no need to resend only to correct them.
>
> I agree on all your comments and your proposed amendment further down
> looks good.

Thanks.

> Should the test code contain the explanations you've written in this email?

No, I don't think so.

I was just showing that thinking aloud while reviewing would be a good way to do a review. The practice

 (1) makes sure that the reviewer actually understood what the patch
     wanted to do (and the reviewee can point out misunderstandings
     if there are any); and

 (2) shows others that the reviewer actually read the patch ;-).

The latter is primarily meant for other people who review the patches. I want to see people get in the habit of responding with something more than just a single-liner "Reviewed-by:", which I often have hard time guessing if the reviewer really read the patch, or just skimmed without spending effort to spot issues, and this message was my attempt to lead with an example.

Will squash in the fix-up in the message you are responding to. Let's move the topic to 'next' after that.

Thanks.