Re: [PATCH] Update diff-highlight

From: Peter Dave Hello <hsu@peterdavehello.org>
Date: Feb 22, 2016, 05:59 UTC
Message-ID: <CA+boQ6Y4223NY_mXHPNR7hpxiepgoSmbM8c6bJxirxTMDVQ8Bg@mail.gmail.com>
In-Reply-To: <CAPig+cRRX4k8jR02YOGTGHUnbOmhHF0oMc-6aCOWuMOpi+BG0A@mail.gmail.com>

Hello Eric,

Thanks for your review and prompt reply, this is my first PR to git, I'll try to update it to follow the conventions.

Best, Peter

--

Now you can follow me on twitter or GitHub :D

2016-02-22 12:49 GMT+08:00 Eric Sunshine <sunshine@sunshineco.com>:

Show 46 quoted lines

> On Sun, Feb 21, 2016 at 11:14 PM, Peter Dave Hello
> <hsu@peterdavehello.org> wrote:
>> From: Peter Dave Hello <peterdavehello@users.noreply.github.com>
>
> This "From:" line looks suspiciously incorrect. If anything, you'd
> probably want to drop the line altogether or use:
>
>     From: Peter Dave Hello <hsu@peterdavehello.org>
>
>> Update diff-highlight
>
> Patches do indeed "update" the project, but this summary line isn't
> telling us much about intention of this patch. Perhaps rephrase it as:
>
>     contrib/diff-highlight: stop hard-coding perl location
>
>> Use `#!/usr/bin/env perl` instead of `#!/usr/bin/perl`
>>
>> So that it can works on FreeBSD.
>
> s/works/work/
>
> Also, you probably want to combine those two lines into one proper
> sentence rather than having one sentence plus a sentence fragment.
>
> Your Signed-off-by: is missing.
>
> Thanks.
>
>> ---
>>  contrib/diff-highlight/diff-highlight | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
>> index ffefc31..b57b0fd 100755
>> --- a/contrib/diff-highlight/diff-highlight
>> +++ b/contrib/diff-highlight/diff-highlight
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/perl
>> +#!/usr/bin/env perl
>>
>>  use 5.008;
>>  use warnings FATAL => 'all';
>>
>> --
>> https://github.com/git/git/pull/200