threads / patch / 26057

patch, 14 partst7800-difftool.sh: Fix a test failure on Cygwin

Subject: [PATCH 07/14] t7800-difftool.sh: Fix a test failure on Cygwin

## tl;dr

6 messages between Dec 14, 2010 and Dec 18, 2010. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Ramsay Jones· Dec 14, 2010, 18:27 UTC · lore

In particular, test 14 'difftool last flag wins' fails. This is caused by git-difftool.perl passing both GIT_DIFFTOOL_NO_PROMPT (='true') and GIT_DIFFTOOL_PROMPT (='true') to the difftool helper script. Despite the appropriate key being deleted from the ENV hash, it seems that once a key has been set in the hash, it gets passed along to the system() call. (ie deleting the key does not do the equivalent of unsetenv()).

In order to fix the problem, we keep track of the required prompt state while processing the arguments, and then set the relevant ENV hash key only once at the end.

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
 git-difftool.perl |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)
Show changes to git-difftool.perl +8 −4
diff --git a/git-difftool.perl b/git-difftool.perl
index e95e4ad..ced1615 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -52,6 +52,7 @@ sub generate_command
 	my @command = (exe('git'), 'diff');
 	my $skip_next = 0;
 	my $idx = -1;
+	my $prompt = '';
 	for my $arg (@ARGV) {
 		$idx++;
 		if ($skip_next) {
@@ -89,13 +90,11 @@ sub generate_command
 			next;
 		}
 		if ($arg eq '-y' || $arg eq '--no-prompt') {
-			$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
-			delete $ENV{GIT_DIFFTOOL_PROMPT};
+			$prompt = 'no';
 			next;
 		}
 		if ($arg eq '--prompt') {
-			$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
-			delete $ENV{GIT_DIFFTOOL_NO_PROMPT};
+			$prompt = 'yes';
 			next;
 		}
 		if ($arg eq '-h' || $arg eq '--help') {
@@ -103,6 +102,11 @@ sub generate_command
 		}
 		push @command, $arg;
 	}
+	if ($prompt eq 'yes') {
+		$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
+	} elsif ($prompt eq 'no') {
+		$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
+	}
 	return @command
 }
 
-- 
1.7.3
Junio C Hamano· Dec 14, 2010, 19:15 UTC · re: Ramsay Jones · lore

Re: [PATCH 07/14] t7800-difftool.sh: Fix a test failure on Cygwin

This is not just "test failure" but can cause a failure in the real life for people who want "the last flag wins" to work, no?

I'll retitle the patch and queue it separately from other test fixes.
Thanks.
Ramsay Jones· Dec 16, 2010, 20:04 UTC · re: Junio C Hamano · lore

Re: [PATCH 07/14] t7800-difftool.sh: Fix a test failure on Cygwin

Junio C Hamano wrote:
> This is not just "test failure" but can cause a failure in the real life
> for people who want "the last flag wins" to work, no?

Yes, indeed. I was concentrating on fixing test-suite failures (and that can mean you have to modify the test, the application or both) so...

> I'll retitle the patch and queue it separately from other test fixes.
Thanks!

ATB, Ramsay Jones

David Aguilar· Dec 17, 2010, 02:33 UTC · re: Ramsay Jones · lore

Re: [PATCH 07/14] t7800-difftool.sh: Fix a test failure on Cygwin

On Tue, Dec 14, 2010 at 06:27:48PM +0000, Ramsay Jones wrote:
Show 27 quoted lines
> 
> In particular, test 14 'difftool last flag wins' fails. This is
> caused by git-difftool.perl passing both GIT_DIFFTOOL_NO_PROMPT
> (='true') and GIT_DIFFTOOL_PROMPT (='true') to the difftool helper
> script. Despite the appropriate key being deleted from the ENV
> hash, it seems that once a key has been set in the hash, it gets
> passed along to the system() call. (ie deleting the key does not
> do the equivalent of unsetenv()).
> 
> In order to fix the problem, we keep track of the required prompt
> state while processing the arguments, and then set the relevant
> ENV hash key only once at the end.
> 
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
>  git-difftool.perl |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/git-difftool.perl b/git-difftool.perl
> index e95e4ad..ced1615 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -52,6 +52,7 @@ sub generate_command
>  	my @command = (exe('git'), 'diff');
>  	my $skip_next = 0;
>  	my $idx = -1;
> +	my $prompt = '';

Would it be simpler to set $prompt = 1 and then flip it to 0 when -y | or --no-prompt is supplied?

Show 24 quoted lines
>  	for my $arg (@ARGV) {
>  		$idx++;
>  		if ($skip_next) {
> @@ -89,13 +90,11 @@ sub generate_command
>  			next;
>  		}
>  		if ($arg eq '-y' || $arg eq '--no-prompt') {
> -			$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
> -			delete $ENV{GIT_DIFFTOOL_PROMPT};
> +			$prompt = 'no';
>  			next;
>  		}
>  		if ($arg eq '--prompt') {
> -			$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
> -			delete $ENV{GIT_DIFFTOOL_NO_PROMPT};
> +			$prompt = 'yes';
>  			next;
>  		}
>  		if ($arg eq '-h' || $arg eq '--help') {
> @@ -103,6 +102,11 @@ sub generate_command
>  		}
>  		push @command, $arg;
>  	}
> +	if ($prompt eq 'yes') {
This would become:
	if ($prompt) {
		...
	}
	else {
		...
	}
Show 11 quoted lines
> +		$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
> +	} elsif ($prompt eq 'no') {
> +		$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
> +	}
>  	return @command
>  }
>  
> -- 
> 1.7.3
> 
> 
-- 
		David
Junio C Hamano· Dec 17, 2010, 19:59 UTC · re: David Aguilar · lore

Re: [PATCH 07/14] t7800-difftool.sh: Fix a test failure on Cygwin

David Aguilar <davvid@gmail.com> writes:
Show 22 quoted lines
>> diff --git a/git-difftool.perl b/git-difftool.perl
>> index e95e4ad..ced1615 100755
>> --- a/git-difftool.perl
>> +++ b/git-difftool.perl
>> @@ -52,6 +52,7 @@ sub generate_command
>>  	my @command = (exe('git'), 'diff');
>>  	my $skip_next = 0;
>>  	my $idx = -1;
>> +	my $prompt = '';
>
> Would it be simpler to set $prompt = 1 and then
> flip it to 0 when -y | or --no-prompt is supplied?
> ...
>
> This would become:
>
> 	if ($prompt) {
> 		...
> 	}
> 	else {
> 		...
> 	}
But that is not what the patch does, is it?

Isn't it more like initializing $prompt to undef and then the above condition becomes:

	if (!defined $prompt) {
        	; # nothing
        elsif ($prompt) {
		setenv PROMPT true
	} else {
		setenv NO_PROMPT true
	}
no?
Ramsay Jones· Dec 18, 2010, 20:32 UTC · re: Junio C Hamano · lore

Re: [PATCH 07/14] t7800-difftool.sh: Fix a test failure on Cygwin

Junio C Hamano wrote:
Show 38 quoted lines
> David Aguilar <davvid@gmail.com> writes:
> 
>>> diff --git a/git-difftool.perl b/git-difftool.perl
>>> index e95e4ad..ced1615 100755
>>> --- a/git-difftool.perl
>>> +++ b/git-difftool.perl
>>> @@ -52,6 +52,7 @@ sub generate_command
>>>  	my @command = (exe('git'), 'diff');
>>>  	my $skip_next = 0;
>>>  	my $idx = -1;
>>> +	my $prompt = '';
>> Would it be simpler to set $prompt = 1 and then
>> flip it to 0 when -y | or --no-prompt is supplied?
>> ...
>>
>> This would become:
>>
>> 	if ($prompt) {
>> 		...
>> 	}
>> 	else {
>> 		...
>> 	}
> 
> But that is not what the patch does, is it?
> 
> Isn't it more like initializing $prompt to undef and then the above
> condition becomes:
> 
> 	if (!defined $prompt) {
>         	; # nothing
>         elsif ($prompt) {
> 		setenv PROMPT true
> 	} else {
> 		setenv NO_PROMPT true
> 	}
> 
> no?

Yes. :-P At least that is what I was aiming for.

In particular, if none of -y, --no-prompt or --prompt is passed on the command-line, I don't want to set either key in the hash. (Or, to put it another way, I don't want to change the behaviour in this case, where neither variable is set in the environment of the helper script)

ATB, Ramsay Jones

← back to recent threads