| Message ID | 8159cbd1b8025f33fb9d0e254db1a3c2a066f853.1540923993.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DiffHighlight.pm: Use correct /dev/null for UNIX and Windows | expand |
On Tue, Oct 30, 2018 at 11:26:36AM -0700, chris via GitGitGadget wrote: > From: chris <chris@webstech.net> You might want to adjust your user.name. :) > Use File::Spec->devnull() for output redirection to avoid messages > when Windows version of Perl is first in path. The message 'The > system cannot find the path specified.' is displayed each time git is > run to get colors. Thanks, makes sense, and the patch looks good to me. -Peff
Jeff King <peff@peff.net> writes: > On Tue, Oct 30, 2018 at 11:26:36AM -0700, chris via GitGitGadget wrote: > >> From: chris <chris@webstech.net> > > You might want to adjust your user.name. :) Yes, absolutely. We'd want to see that the From: line and one of the Signed-off-by: lines are idential. >> Use File::Spec->devnull() for output redirection to avoid messages >> when Windows version of Perl is first in path. The message 'The >> system cannot find the path specified.' is displayed each time git is >> run to get colors. > > Thanks, makes sense, and the patch looks good to me. Yup, and we already use File::Spec everywhere anyway, so this is not a new dependency, either. Which is very good.
"chris via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: chris <chris@webstech.net> > > Use File::Spec->devnull() for output redirection to avoid messages > when Windows version of Perl is first in path. The message 'The Dscho, "Windows version of Perl is first in path" somehow feels contradicting with what one of the topics I saw from you were trying to enforce (or, at least, "set as the supported configuration"). I am guessing that the Perl you are building and shipping with Git for Windows would yield what the shell that ends up running the scriptlet `git config --get-color $key` prefers when asked for File::Spec->devnull(), and nothing will break with this patch even if that is "/dev/null", but I thought I'd double check. Thanks. > system cannot find the path specified.' is displayed each time git is > run to get colors. > > Signed-off-by: Chris. Webster <chris@webstech.net> > --- > contrib/diff-highlight/DiffHighlight.pm | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm > index 536754583..7440aa1c4 100644 > --- a/contrib/diff-highlight/DiffHighlight.pm > +++ b/contrib/diff-highlight/DiffHighlight.pm > @@ -4,6 +4,11 @@ use 5.008; > use warnings FATAL => 'all'; > use strict; > > +# Use the correct value for both UNIX and Windows (/dev/null vs nul) > +use File::Spec; > + > +my $NULL = File::Spec->devnull(); > + > # Highlight by reversing foreground and background. You could do > # other things like bold or underline if you prefer. > my @OLD_HIGHLIGHT = ( > @@ -134,7 +139,7 @@ sub highlight_stdin { > # fallback, which means we will work even if git can't be run. > sub color_config { > my ($key, $default) = @_; > - my $s = `git config --get-color $key 2>/dev/null`; > + my $s = `git config --get-color $key 2>$NULL`; > return length($s) ? $s : $default; > }
> Subject: Re: [PATCH 1/1] Use correct /dev/null for UNIX and Windows As this is only about contrib/diff-highlight, please make it clear that it is the area the patch affects on its title, i.e. Subject: diff-highlight: use File::Spec->devnull(), not /dev/null or something like that. > From: chris <chris@webstech.net> Please make this line read like From: Chris Webster <chris@webstech.net> i.e. the author should be the person who is signing off that patch. > Use File::Spec->devnull() for output redirection to avoid messages > when Windows version of Perl is first in path. The message 'The > system cannot find the path specified.' is displayed each time git is > run to get colors. > > Signed-off-by: Chris. Webster <chris@webstech.net> > --- > contrib/diff-highlight/DiffHighlight.pm | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) There are a handful more instances of /dev/null found if you do $ git grep /dev/null -- \*.pl \*.pm The one in perl/Git.pm must be shared by scripts written in Perl, so it may be worth giving the same tweak to it, like this patch does to the highlight script. > diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm > index 536754583..7440aa1c4 100644 > --- a/contrib/diff-highlight/DiffHighlight.pm > +++ b/contrib/diff-highlight/DiffHighlight.pm > @@ -4,6 +4,11 @@ use 5.008; > use warnings FATAL => 'all'; > use strict; > > +# Use the correct value for both UNIX and Windows (/dev/null vs nul) > +use File::Spec; > + > +my $NULL = File::Spec->devnull(); > + > # Highlight by reversing foreground and background. You could do > # other things like bold or underline if you prefer. > my @OLD_HIGHLIGHT = ( > @@ -134,7 +139,7 @@ sub highlight_stdin { > # fallback, which means we will work even if git can't be run. > sub color_config { > my ($key, $default) = @_; > - my $s = `git config --get-color $key 2>/dev/null`; > + my $s = `git config --get-color $key 2>$NULL`; > return length($s) ? $s : $default; > }
Resending in text mode. On Tue, Oct 30, 2018 at 10:20 PM Chris Webster <chris@webstech.net> wrote: > > On Tue, Oct 30, 2018 at 9:54 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> "chris via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >> > From: chris <chris@webstech.net> >> > >> > Use File::Spec->devnull() for output redirection to avoid messages >> > when Windows version of Perl is first in path. The message 'The >> >> Dscho, "Windows version of Perl is first in path" somehow feels >> contradicting with what one of the topics I saw from you were trying >> to enforce (or, at least, "set as the supported configuration"). >> >> I am guessing that the Perl you are building and shipping with Git >> for Windows would yield what the shell that ends up running the >> scriptlet `git config --get-color $key` prefers when asked for >> File::Spec->devnull(), and nothing will break with this patch even >> if that is "/dev/null", but I thought I'd double check. >> >> Thanks. >> This problem originally showed up in the https://github.com/so-fancy/diff-so-fancy project, which has a copy of DiffHighlight.pm. That project allows diffsofancy (perl) to be run from the command line without requiring the bash environment ((well , sort of) including the associated perl). > >> > system cannot find the path specified.' is displayed each time git is >> > run to get colors. >> > >> > Signed-off-by: Chris. Webster <chris@webstech.net> >> > --- >> > contrib/diff-highlight/DiffHighlight.pm | 7 ++++++- >> > 1 file changed, 6 insertions(+), 1 deletion(-) >> > >> > diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm >> > index 536754583..7440aa1c4 100644 >> > --- a/contrib/diff-highlight/DiffHighlight.pm >> > +++ b/contrib/diff-highlight/DiffHighlight.pm >> > @@ -4,6 +4,11 @@ use 5.008; >> > use warnings FATAL => 'all'; >> > use strict; >> > >> > +# Use the correct value for both UNIX and Windows (/dev/null vs nul) >> > +use File::Spec; >> > + >> > +my $NULL = File::Spec->devnull(); >> > + >> > # Highlight by reversing foreground and background. You could do >> > # other things like bold or underline if you prefer. >> > my @OLD_HIGHLIGHT = ( >> > @@ -134,7 +139,7 @@ sub highlight_stdin { >> > # fallback, which means we will work even if git can't be run. >> > sub color_config { >> > my ($key, $default) = @_; >> > - my $s = `git config --get-color $key 2>/dev/null`; >> > + my $s = `git config --get-color $key 2>$NULL`; >> > return length($s) ? $s : $default; >> > }
Chris Webster <chris@webstech.net> writes: >>> > Use File::Spec->devnull() for output redirection to avoid messages >>> > when Windows version of Perl is first in path. The message 'The >>> >>> Dscho, "Windows version of Perl is first in path" somehow feels >>> contradicting with what one of the topics I saw from you were trying >>> to enforce (or, at least, "set as the supported configuration"). >>> >>> I am guessing that the Perl you are building and shipping with Git >>> for Windows would yield what the shell that ends up running the >>> scriptlet `git config --get-color $key` prefers when asked for >>> File::Spec->devnull(), and nothing will break with this patch even >>> if that is "/dev/null", but I thought I'd double check. >>> >>> Thanks. >>> > This problem originally showed up in the > https://github.com/so-fancy/diff-so-fancy project, which has a copy of > DiffHighlight.pm. That project allows diffsofancy (perl) to be run > from the command line without requiring the bash environment ((well , > sort of) including the associated perl). Thanks for additional comments. In any case, Windows is not my bailiwick, so I'll hope that the above comments from you would help Dscho in his response and wait. I know use of File::Spec->devnull() won't hurt POSIX folks so making sure this won't break Git for Windows is the primary thing I woudl worry about this patch.
Hi Junio, On Wed, 31 Oct 2018, Junio C Hamano wrote: > Chris Webster <chris@webstech.net> writes: > > >>> > Use File::Spec->devnull() for output redirection to avoid messages > >>> > when Windows version of Perl is first in path. The message 'The > >>> > >>> Dscho, "Windows version of Perl is first in path" somehow feels > >>> contradicting with what one of the topics I saw from you were trying > >>> to enforce (or, at least, "set as the supported configuration"). > >>> > >>> I am guessing that the Perl you are building and shipping with Git > >>> for Windows would yield what the shell that ends up running the > >>> scriptlet `git config --get-color $key` prefers when asked for > >>> File::Spec->devnull(), and nothing will break with this patch even > >>> if that is "/dev/null", but I thought I'd double check. > >>> > >>> Thanks. > >>> > > This problem originally showed up in the > > https://github.com/so-fancy/diff-so-fancy project, which has a copy of > > DiffHighlight.pm. That project allows diffsofancy (perl) to be run > > from the command line without requiring the bash environment ((well , > > sort of) including the associated perl). > > Thanks for additional comments. > > In any case, Windows is not my bailiwick, so I'll hope that the > above comments from you would help Dscho in his response and wait. > I know use of File::Spec->devnull() won't hurt POSIX folks so making > sure this won't break Git for Windows is the primary thing I woudl > worry about this patch. Indeed, the patch in question regards something I consider outside Git for Windows' realm. As Chris said, you can run this script from a PowerShell prompt, without any Git Bash (and without Git's Perl) involved. I am fine with this patch, as long as the author name is fixed to match the name in the Signed-off-by: footer ;-) [*1*] Ciao, Dscho Footnote *1*: This patch came in via GitGitGadget, and if I had infinite amounts of time, I would probably implement some rudimentary pre-checks, such as: does the Author: header match the first Signed-off-by: footer, is the commit message wrapped correctly, does the oneline have a prefix and continues lower-case, etc. And GitGitGadget would then point out the issues, possibly even try to fix them and push up the fixed commits. If anybody agrees with these goals and is curious enough to dive into some Typescript programming, I'd be very happy to guide that person through implementing this ;-)
Hi, On Wed, 31 Oct 2018, Junio C Hamano wrote: > > From: chris <chris@webstech.net> > > Please make this line read like > > From: Chris Webster <chris@webstech.net> > > i.e. the author should be the person who is signing off that patch. This is most likely recorded as the commit's author in the commit object... Chris, to fix it, make sure that your `user.name` is configured correctly, and then call `git commit --amend --reset-author`. > > Use File::Spec->devnull() for output redirection to avoid messages > > when Windows version of Perl is first in path. The message 'The > > system cannot find the path specified.' is displayed each time git is > > run to get colors. > > > > Signed-off-by: Chris. Webster <chris@webstech.net> > > --- > > contrib/diff-highlight/DiffHighlight.pm | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > There are a handful more instances of /dev/null found if you do > > $ git grep /dev/null -- \*.pl \*.pm > > The one in perl/Git.pm must be shared by scripts written in Perl, so > it may be worth giving the same tweak to it, like this patch does to > the highlight script. I do not think that perl/Git.pm is intended to run with any random Perl interpreter. It has to be one that has been verified to work correctly with the Perl code in perl/, and that code is notoriously reliant on POSIX behavior, hence our choice to go with MSYS2 Perl (there *is* a MINGW Perl package in Git for Windows' SDK, but it will most likely not work, in particular because of the missing Subversion bindings). So I would restrict the search to contrib/\*.pl, contrib/\*.perl and contrib/\*.pm. The stuff in contrib/ is supposed to be semi-independent from the particular Git one is using (and from whatever Perl is shipped with it, if any). Ciao, Johannes > > diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm > > index 536754583..7440aa1c4 100644 > > --- a/contrib/diff-highlight/DiffHighlight.pm > > +++ b/contrib/diff-highlight/DiffHighlight.pm > > @@ -4,6 +4,11 @@ use 5.008; > > use warnings FATAL => 'all'; > > use strict; > > > > +# Use the correct value for both UNIX and Windows (/dev/null vs nul) > > +use File::Spec; > > + > > +my $NULL = File::Spec->devnull(); > > + > > # Highlight by reversing foreground and background. You could do > > # other things like bold or underline if you prefer. > > my @OLD_HIGHLIGHT = ( > > @@ -134,7 +139,7 @@ sub highlight_stdin { > > # fallback, which means we will work even if git can't be run. > > sub color_config { > > my ($key, $default) = @_; > > - my $s = `git config --get-color $key 2>/dev/null`; > > + my $s = `git config --get-color $key 2>$NULL`; > > return length($s) ? $s : $default; > > } >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Indeed, the patch in question regards something I consider outside Git for > Windows' realm. As Chris said, you can run this script from a PowerShell > prompt, without any Git Bash (and without Git's Perl) involved. > > I am fine with this patch, as long as the author name is fixed to match > the name in the Signed-off-by: footer ;-) [*1*] Thanks, I'll find a corrected patch on the list (or manufacture it out of the original) and queue, then.
diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm index 536754583..7440aa1c4 100644 --- a/contrib/diff-highlight/DiffHighlight.pm +++ b/contrib/diff-highlight/DiffHighlight.pm @@ -4,6 +4,11 @@ use 5.008; use warnings FATAL => 'all'; use strict; +# Use the correct value for both UNIX and Windows (/dev/null vs nul) +use File::Spec; + +my $NULL = File::Spec->devnull(); + # Highlight by reversing foreground and background. You could do # other things like bold or underline if you prefer. my @OLD_HIGHLIGHT = ( @@ -134,7 +139,7 @@ sub highlight_stdin { # fallback, which means we will work even if git can't be run. sub color_config { my ($key, $default) = @_; - my $s = `git config --get-color $key 2>/dev/null`; + my $s = `git config --get-color $key 2>$NULL`; return length($s) ? $s : $default; }