From: Josh Steadmon <steadmon@google.com>
To: Kyle Meyer <kyle@kyleam.com>
Cc: git@vger.kernel.org, Yaroslav O Halchenko <debian@onerussian.com>
Subject: Re: [PATCH] submodule add: show 'add --dry-run' stderr when aborting
Date: Wed, 8 Jan 2020 13:41:36 -0800 [thread overview]
Message-ID: <20200108214136.GB63040@google.com> (raw)
In-Reply-To: <20200108003121.28034-1-kyle@kyleam.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!
next prev parent reply other threads:[~2020-01-08 21:41 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-08 0:31 [PATCH] submodule add: show 'add --dry-run' stderr when aborting Kyle Meyer
2020-01-08 21:41 ` Josh Steadmon [this message]
2020-01-09 2:39 ` Kyle Meyer
2020-01-16 16:17 ` Johannes Schindelin
2020-01-16 20:23 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200108214136.GB63040@google.com \
--to=steadmon@google.com \
--cc=debian@onerussian.com \
--cc=git@vger.kernel.org \
--cc=kyle@kyleam.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).