diff mbox series

[v3,7/7] add--interactive.perl: use add--helper --show-help for help_cmd

Message ID b9a1a7e37a477e978f19cbcc9b41f80519de54da.1548062019.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Turn git add-i into built-in | expand

Commit Message

Linus Arver via GitGitGadget Jan. 21, 2019, 9:13 a.m. UTC 
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(-)

Comments

Ævar Arnfjörð Bjarmason Jan. 21, 2019, 9:59 a.m. UTC | #1 
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.

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";
+					}
+				}
 			}
 		}
 	}
Slavica Djukic Jan. 21, 2019, 11:59 a.m. UTC | #2 
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";
> +					}
> +				}
>   			}
>   		}
>   	}
>
diff mbox series

Patch

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 {