Re: [PATCH] submodule add: show 'add --dry-run' stderr when aborting

[Josh Steadmon <steadmon@google.com>]

On 2020.01.07 19:31, Kyle Meyer wrote:
> Unless --force is specified, 'submodule add' checks if the destination
> path is ignored by calling 'git add --dry-run --ignore-missing', and,
> if that call fails, aborts with a custom "path is ignored" message (a
> slight variant of what 'git add' shows).  Aborting early rather than
> letting the downstream 'git add' call fail is done so that the command
> exits before cloning into the destination path.  However, in rare
> cases where the dry-run call fails for a reason other than the path
> being ignored---for example, due to a preexisting index.lock
> file---displaying the "ignored path" error message hides the real
> source of the failure.
> 
> Instead of displaying the tailored "ignored path" message, let's
> report the standard error from the dry run to give the caller more
> accurate information about failures that are not due to an ignored
> path.
> 
> For the ignored path case, this leads to the following change in the
> error message:
> 
>   The following [-path is-]{+paths are+} ignored by one of your .gitignore files:
>   <destination path>
>   Use -f if you really want to add [-it.-]{+them.+}
> 
> The new phrasing is a bit awkward, because 'submodule add' is only
> dealing with one destination path.  Alternatively, we could continue
> to use the tailored message when the exit code is 1 (the expected
> status for a failure due to an ignored path) and relay the standard
> error for all other non-zero exits.  That, however, risks hiding the
> message of unrelated failures that share an exit code of 1, so it
> doesn't seem worth doing just to avoid a clunkier, but still clear,
> error message.
> 
> Signed-off-by: Kyle Meyer <kyle@kyleam.com>
> ---
>  git-submodule.sh           | 14 ++++++++------
>  t/t7400-submodule-basic.sh | 15 +++++++++++++--
>  2 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/git-submodule.sh b/git-submodule.sh
> index aaa1809d24..afcb4c0948 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -241,13 +241,15 @@ cmd_add()
>  	    die "$(eval_gettext "'\$sm_path' does not have a commit checked out")"
>  	fi
>  
> -	if test -z "$force" &&
> -		! git add --dry-run --ignore-missing --no-warn-embedded-repo "$sm_path" > /dev/null 2>&1
> +	if test -z "$force"
>  	then
> -		eval_gettextln "The following path is ignored by one of your .gitignore files:
> -\$sm_path
> -Use -f if you really want to add it." >&2
> -		exit 1
> +	    dryerr=$(git add --dry-run --ignore-missing --no-warn-embedded-repo "$sm_path" 2>&1 >/dev/null)
> +	    res=$?
> +	    if test $res -ne 0
> +	    then
> +		 echo >&2 "$dryerr"
> +		 exit $res
> +	    fi
>  	fi
>  
>  	if test -n "$custom_name"

Seems reasonable: we move the dry-run add inside the if block, and
capture its stderr, then display the message only if the return code is
non-zero.


> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 7f75bb1be6..42a00f95b9 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -156,9 +156,9 @@ test_expect_success 'submodule add to .gitignored path fails' '
>  	(
>  		cd addtest-ignore &&
>  		cat <<-\EOF >expect &&
> -		The following path is ignored by one of your .gitignore files:
> +		The following paths are ignored by one of your .gitignore files:
>  		submod
> -		Use -f if you really want to add it.
> +		Use -f if you really want to add them.
>  		EOF
>  		# Does not use test_commit due to the ignore
>  		echo "*" > .gitignore &&
> @@ -191,6 +191,17 @@ test_expect_success 'submodule add to reconfigure existing submodule with --forc
>  	)
>  '
>  
> +test_expect_success 'submodule add relays add --dry-run stderr' '
> +	test_when_finished "rm -rf addtest/.git/index.lock" &&
> +	(
> +		cd addtest &&
> +		: >.git/index.lock &&
> +		! git submodule add "$submodurl" sub-while-locked 2>output.err &&
> +		test_i18ngrep "^fatal: .*index\.lock" output.err &&
> +		test_path_is_missing sub-while-locked
> +	)
> +'
> +
>  test_expect_success 'submodule add --branch' '
>  	echo "refs/heads/initial" >expect-head &&
>  	cat <<-\EOF >expect-heads &&

I had to look up what ":" does, but it looks like it's reasonably widely
used in other tests so that seems fine. However, it looks like you don't
even need the : command and can just ">.git/index.lock" by itself (see
the "setup - initial commit" test case in this file for an example).

> base-commit: 042ed3e048af08014487d19196984347e3be7d1c
> -- 
> 2.24.1
> 


Looks good to me. Thanks for the patch!