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@xxxxxxxxxx>
> ---
> 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!
