From: Slavica Djukic <slavicadj.ip2018@gmail.com>
Date: Mon, 21 Jan 2019 11:59:22 GMT
Subject: Re: [PATCH v3 7/7] add--interactive.perl: use add--helper --show-help for help_cmd
Message-ID: <1224d852-6009-7c91-764a-f1015ed253b5@gmail.com>
In-Reply-To: <87fttm49hv.fsf@evledraar.gmail.com>

Hello Ævar,

thanks for taking time and making review.

On 21-Jan-19 10:59 AM, Ævar Arnfjörð Bjarmason wrote:
> On Mon, Jan 21 2019, Slavica Djukic via GitGitGadget wrote:
>
>> From: Slavica Djukic <slawica92@hotmail.com>
>>
>> Change help_cmd sub in git-add--interactive.perl to use
>> show-help command from builtin add--helper.
>>
>> Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
>> ---
>>   git-add--interactive.perl | 11 +----------
>>   1 file changed, 1 insertion(+), 10 deletions(-)
>>
>> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
>> index a6536f9cf3..32ee729a58 100755
>> --- a/git-add--interactive.perl
>> +++ b/git-add--interactive.perl
>> @@ -1717,16 +1717,7 @@ sub quit_cmd {
>>   }
>>
>>   sub help_cmd {
>> -# TRANSLATORS: please do not translate the command names
>> -# 'status', 'update', 'revert', etc.
>> -	print colored $help_color, __ <<'EOF' ;
>> -status        - show paths with changes
>> -update        - add working tree state to the staged set of changes
>> -revert        - revert staged set of changes back to the HEAD version
>> -patch         - pick hunks and update selectively
>> -diff          - view diff between HEAD and index
>> -add untracked - add contents of untracked files to the staged set of changes
>> -EOF
>> +	system(qw(git add--helper --show-help));
>>   }
>>
>>   sub process_args {
> Both this and an earlier change in this series replaces a callback
> command with an invocation of system() without any error checking. So if
> this add-helper fails for whatever reason we'll silently fail to report
> it.
>
> I think it makes sense to put something like the following WIP code
> earlier in the series. Then if the command was e.g. ["false", "git",
> "status"] we'd see:
>
>      What now> s
>      oh noes when running 'false git status': returned error '1'
>
> WIP patch. Obviously not ready as-is, but feel free to consider this to
> have my SOB & adapt it.


And thank you for writing up this WIP patch.  I will adapt it and have 
your SOB.

-Slavica


>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 20eb81cc92..1cd5f8122b 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1777,7 +1777,7 @@ sub process_args {
>   }
>
>   sub main_loop {
> -	my @cmd = ([ 'status', \&status_cmd, ],
> +	my @cmd = ([ 'status', ["git", "status"] ],
>   		   [ 'update', \&update_cmd, ],
>   		   [ 'revert', \&revert_cmd, ],
>   		   [ 'add untracked', \&add_untracked_cmd, ],
> @@ -1794,11 +1794,23 @@ sub main_loop {
>   					     ON_EOF => \&quit_cmd,
>   					     IMMEDIATE => 1 }, @cmd);
>   		if ($it) {
> -			eval {
> -				$it->[1]->();
> -			};
> -			if ($@) {
> -				print "$@";
> +			my $cb = $it->[1];
> +			if (ref $cb eq 'CODE') {
> +				eval {
> +					$cb->();
> +					1;
> +				} or do {
> +					print "$@";
> +				};
> +			} else {
> +				if (system(@$cb) != 0) {
> +					if ($? == -1 || $? & 127) {
> +						print STDERR "oh noes when running '@$cb': unexpected '$?'\n";
> +					} else {
> +						my $ret = $? >> 8;
> +						print STDERR "oh noes when running '@$cb': returned error '$ret'\n";
> +					}
> +				}
>   			}
>   		}
>   	}
>