[PATCH] MinGW: fix diff --no-index /dev/null ...

[PATCH] MinGW: fix diff --no-index /dev/null ...

[Johannes Schindelin]

When launching "diff --no-index" with a parameter "/dev/null", the MSys
bash converts the "/dev/null" to a "nul", which usually makes sense.  But
diff --no-index got confused and tried to access a _file_ called "nul".

While at it, the comment in t4012, expressed as ":# <text>" was turned
into ": <text>" so that MSys' path name mangling does not kick in.

With this patch, t4012 passes in msysGit.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 diff-no-index.c        |    4 ++++
 t/t4012-diff-binary.sh |    2 +-
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index 0a14268..598687b 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -38,6 +38,10 @@ static int get_mode(const char *path, int *mode)
 
 	if (!path || !strcmp(path, "/dev/null"))
 		*mode = 0;
+#ifdef _WIN32
+	else if (!strcasecmp(path, "nul"))
+		*mode = 0;
+#endif
 	else if (!strcmp(path, "-"))
 		*mode = create_ce_mode(0666);
 	else if (lstat(path, &st))
diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
index 3cf5b5c..f64aa48 100755
--- a/t/t4012-diff-binary.sh
+++ b/t/t4012-diff-binary.sh
@@ -87,7 +87,7 @@ nul_to_q() {
 
 test_expect_success 'diff --no-index with binary creation' '
 	echo Q | q_to_nul >binary &&
-	(:# hide error code from diff, which just indicates differences
+	(: hide error code from diff, which just indicates differences
 	 git diff --binary --no-index /dev/null binary >current ||
 	 true
 	) &&
-- 
1.6.2.327.g0fa6c

Re: [PATCH] MinGW: fix diff --no-index /dev/null ...

[Johannes Sixt]

On Samstag, 7. März 2009, Johannes Schindelin wrote:
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -38,6 +38,10 @@ static int get_mode(const char *path, int *mode)
>
>  	if (!path || !strcmp(path, "/dev/null"))
>  		*mode = 0;
> +#ifdef _WIN32
> +	else if (!strcasecmp(path, "nul"))
> +		*mode = 0;
> +#endif
>  	else if (!strcmp(path, "-"))
>  		*mode = create_ce_mode(0666);
>  	else if (lstat(path, &st))

IMO this #ifdef is a reasonable compromise. Trying to move this into 
git-compat-util.h or mingw.h somehow would only obfuscate the code.

Tested-by: Johannes Sixt <j6t@kdbg.org>

-- Hannes

Re: [PATCH] MinGW: fix diff --no-index /dev/null ...

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> diff --git a/diff-no-index.c b/diff-no-index.c
> index 0a14268..598687b 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -38,6 +38,10 @@ static int get_mode(const char *path, int *mode)
>  
>  	if (!path || !strcmp(path, "/dev/null"))
>  		*mode = 0;
> +#ifdef _WIN32
> +	else if (!strcasecmp(path, "nul"))
> +		*mode = 0;
> +#endif
>  	else if (!strcmp(path, "-"))
>  		*mode = create_ce_mode(0666);
>  	else if (lstat(path, &st))

I had a brief "Huh? -- doesn't this call for an is_dev_null() helper"
moment, but I think you are right that diff-no-index.c is the right place
to special case it.

Should this go to 'maint'?

> diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
> index 3cf5b5c..f64aa48 100755
> --- a/t/t4012-diff-binary.sh
> +++ b/t/t4012-diff-binary.sh
> @@ -87,7 +87,7 @@ nul_to_q() {
>  
>  test_expect_success 'diff --no-index with binary creation' '
>  	echo Q | q_to_nul >binary &&
> -	(:# hide error code from diff, which just indicates differences
> +	(: hide error code from diff, which just indicates differences
>  	 git diff --binary --no-index /dev/null binary >current ||
>  	 true
>  	) &&
> -- 
> 1.6.2.327.g0fa6c

Re: [PATCH] MinGW: fix diff --no-index /dev/null ...

[Johannes Schindelin]

Hi,

On Sat, 7 Mar 2009, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/diff-no-index.c b/diff-no-index.c
> > index 0a14268..598687b 100644
> > --- a/diff-no-index.c
> > +++ b/diff-no-index.c
> > @@ -38,6 +38,10 @@ static int get_mode(const char *path, int *mode)
> >  
> >  	if (!path || !strcmp(path, "/dev/null"))
> >  		*mode = 0;
> > +#ifdef _WIN32
> > +	else if (!strcasecmp(path, "nul"))
> > +		*mode = 0;
> > +#endif
> >  	else if (!strcmp(path, "-"))
> >  		*mode = create_ce_mode(0666);
> >  	else if (lstat(path, &st))
> 
> I had a brief "Huh? -- doesn't this call for an is_dev_null() helper"
> moment, but I think you are right that diff-no-index.c is the right place
> to special case it.

You're right, it needs to be explained in the commit message.  So how 
about this:

-- snip --
Unlike other places where a string is compared to /dev/null, in this case 
do not parse a diff, but rather command line parameters that have 
possibly been transformed from /dev/null to nul, so that the file can be 
opened.
-- snap --

If you like it, may I ask you to amend the message?

> Should this go to 'maint'?

Technically, yes, as it is a fix.

However, it might not be necessary, as literally all Windows work on Git 
happens in git/mingw.git, git/mingw/j6t.git and git/mingw/4msysgit.git.

Your call.

Thanks,
Dscho

Re: [PATCH] MinGW: fix diff --no-index /dev/null ...

[Johannes Sixt]

On Samstag, 7. März 2009, Junio C Hamano wrote:
> Should this go to 'maint'?

I don't think that's necessary. It fixes only that someone literally writes on 
the Windows command line

   git diff --no-index nul foo.c

when on Unix one would have written

   git diff --no-index /dev/null foo.c

In practice, only the test suite does this ;)

-- Hannes

Re: [PATCH] MinGW: fix diff --no-index /dev/null ...

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Unlike other places where a string is compared to /dev/null, in this case 
> do not parse a diff, but rather command line parameters that have 
> possibly been transformed from /dev/null to nul, so that the file can be 
> opened.
> -- snap --
>
> If you like it, may I ask you to amend the message?
>
>> Should this go to 'maint'?
>
> Technically, yes, as it is a fix.
>
> However, it might not be necessary, as literally all Windows work on Git 
> happens in git/mingw.git, git/mingw/j6t.git and git/mingw/4msysgit.git.

Yeah, I should have been clearer.  Unless msysgit will have a maintenance
track for 1.6.1.X, there is no point.

Thanks for reading my mind and a clarified message I can use for amending.