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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Slavica Djukic via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Phillip Wood <phillip.wood@dunelm.org.uk>,
	Junio C Hamano <gitster@pobox.com>,
	Slavica Djukic <slawica92@hotmail.com>
Subject: Re: [PATCH v3 7/7] add--interactive.perl: use add--helper --show-help for help_cmd
Date: Mon, 21 Jan 2019 10:59:08 +0100	[thread overview]
Message-ID: <87fttm49hv.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <b9a1a7e37a477e978f19cbcc9b41f80519de54da.1548062019.git.gitgitgadget@gmail.com>


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";
+					}
+				}
 			}
 		}
 	}
