Re: [PATCH] Update diff-highlight

[Roberto Tyley]

On 22 February 2016 at 04:49, Eric Sunshine  wrote:
> On Sun, Feb 21, 2016 at 11:14 PM, Peter Dave Hello
>  wrote:
>> From: Peter Dave Hello 
>
> This "From:" line looks suspiciously incorrect. If anything, you'd
> probably want to drop the line altogether or use:
>
> From: Peter Dave Hello 

Peter's commit (https://github.com/git/git/commit/15415c6e) had an author of
'peterdavehe...@users.noreply.github.com' (perhaps because the commit was
generated through GitHub's interface?), and submitGit added it as an in-body
'From: ' line because it differed from the address used to send the email
(h...@peterdavehello.org - submitGit always uses the user's
primary-email-address-in-GitHub to send the email).

A 'noreply' address is obviously not wanted in this context though, so
I've updated
submitGit to disregard them when deciding whether or not to generate an in-body
'From: ' header: https://github.com/rtyley/submitgit/pull/29

Roberto
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Update diff-highlight

[Junio C Hamano]

Peter Dave Hello  writes:

> From: Peter Dave Hello 
>
> Use `#!/usr/bin/env perl` instead of `#!/usr/bin/perl`

Even though there are existing examples in contrib/ parts that use
this pattern, we try to avoid use of #!/usr/bin/env in more serious
parts of our system.  This is to control the exact interpreter used
to run our scripts at the build time, and to avoid interference by
end users' $PATH environment.  So adding more use of #!/usr/bin/env
is not quite a welcome move, even if it is to contrib/ part.

Perhaps you can instead mimick the way how contrib/subtree uses the
same SHELL_PATH used in the primary Makefile (and config.mak) to
turn git-subtree.sh into git-subtree command.  Rename the source to
diff-highlight.perl, and use PERL_PATH when build procedure turns it
into diff-highlight?

I think that is more in line with the rest of the system.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Update diff-highlight

[Peter Dave Hello]

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 :
> On Sun, Feb 21, 2016 at 11:14 PM, Peter Dave Hello
>  wrote:
>> From: Peter Dave Hello 
>
> This "From:" line looks suspiciously incorrect. If anything, you'd
> probably want to drop the line altogether or use:
>
> From: Peter Dave Hello 
>
>> 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Update diff-highlight

[Eric Sunshine]

On Sun, Feb 21, 2016 at 11:14 PM, Peter Dave Hello
 wrote:
> From: Peter Dave Hello 

This "From:" line looks suspiciously incorrect. If anything, you'd
probably want to drop the line altogether or use:

From: Peter Dave Hello 

> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Update diff-highlight

[Peter Dave Hello]

From: Peter Dave Hello 

Use `#!/usr/bin/env perl` instead of `#!/usr/bin/perl`

So that it can works on FreeBSD.
---
 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html