{"thread":{"id":"52589","subject":"[PATCH] submodule add: show 'add --dry-run' stderr when aborting","startedAt":"2020-01-08T00:31:40Z","lastAt":"2020-01-16T20:23:17Z","messageCount":5,"participants":["Kyle Meyer","Josh Steadmon","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"389437","messageId":"20200108003121.28034-1-kyle@kyleam.com","threadId":"52589","inReplyTo":null,"subject":"[PATCH] submodule add: show 'add --dry-run' stderr when aborting","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2020-01-08T00:31:21Z","receivedAt":"2020-01-08T00:31:40Z","isPatch":true,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Unless --force is specified, 'submodule add' checks if the destination\npath is ignored by calling 'git add --dry-run --ignore-missing', and,\nif that call fails, aborts with a custom \"path is ignored\" message (a\nslight variant of what 'git add' shows).  Aborting early rather than\nletting the downstream 'git add' call fail is done so that the command\nexits before cloning into the destination path.  However, in rare\ncases where the dry-run call fails for a reason other than the path\nbeing ignored---for example, due to a preexisting index.lock\nfile---displaying the \"ignored path\" error message hides the real\nsource of the failure.\n\nInstead of displaying the tailored \"ignored path\" message, let's\nreport the standard error from the dry run to give the caller more\naccurate information about failures that are not due to an ignored\npath.\n\nFor the ignored path case, this leads to the following change in the\nerror message:\n\n  The following [-path is-]{+paths are+} ignored by one of your .gitignore files:\n  <destination path>\n  Use -f if you really want to add [-it.-]{+them.+}\n\nThe new phrasing is a bit awkward, because 'submodule add' is only\ndealing with one destination path.  Alternatively, we could continue\nto use the tailored message when the exit code is 1 (the expected\nstatus for a failure due to an ignored path) and relay the standard\nerror for all other non-zero exits.  That, however, risks hiding the\nmessage of unrelated failures that share an exit code of 1, so it\ndoesn't seem worth doing just to avoid a clunkier, but still clear,\nerror message.\n\nSigned-off-by: Kyle Meyer <kyle@kyleam.com>\n---\n git-submodule.sh           | 14 ++++++++------\n t/t7400-submodule-basic.sh | 15 +++++++++++++--\n 2 files changed, 21 insertions(+), 8 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex aaa1809d24..afcb4c0948 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -241,13 +241,15 @@ cmd_add()\n \t    die \"$(eval_gettext \"'\\$sm_path' does not have a commit checked out\")\"\n \tfi\n \n-\tif test -z \"$force\" &&\n-\t\t! git add --dry-run --ignore-missing --no-warn-embedded-repo \"$sm_path\" > /dev/null 2>&1\n+\tif test -z \"$force\"\n \tthen\n-\t\teval_gettextln \"The following path is ignored by one of your .gitignore files:\n-\\$sm_path\n-Use -f if you really want to add it.\" >&2\n-\t\texit 1\n+\t    dryerr=$(git add --dry-run --ignore-missing --no-warn-embedded-repo \"$sm_path\" 2>&1 >/dev/null)\n+\t    res=$?\n+\t    if test $res -ne 0\n+\t    then\n+\t\t echo >&2 \"$dryerr\"\n+\t\t exit $res\n+\t    fi\n \tfi\n \n \tif test -n \"$custom_name\"\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 7f75bb1be6..42a00f95b9 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -156,9 +156,9 @@ test_expect_success 'submodule add to .gitignored path fails' '\n \t(\n \t\tcd addtest-ignore &&\n \t\tcat <<-\\EOF >expect &&\n-\t\tThe following path is ignored by one of your .gitignore files:\n+\t\tThe following paths are ignored by one of your .gitignore files:\n \t\tsubmod\n-\t\tUse -f if you really want to add it.\n+\t\tUse -f if you really want to add them.\n \t\tEOF\n \t\t# Does not use test_commit due to the ignore\n \t\techo \"*\" > .gitignore &&\n@@ -191,6 +191,17 @@ test_expect_success 'submodule add to reconfigure existing submodule with --forc\n \t)\n '\n \n+test_expect_success 'submodule add relays add --dry-run stderr' '\n+\ttest_when_finished \"rm -rf addtest/.git/index.lock\" &&\n+\t(\n+\t\tcd addtest &&\n+\t\t: >.git/index.lock &&\n+\t\t! git submodule add \"$submodurl\" sub-while-locked 2>output.err &&\n+\t\ttest_i18ngrep \"^fatal: .*index\\.lock\" output.err &&\n+\t\ttest_path_is_missing sub-while-locked\n+\t)\n+'\n+\n test_expect_success 'submodule add --branch' '\n \techo \"refs/heads/initial\" >expect-head &&\n \tcat <<-\\EOF >expect-heads &&\n\nbase-commit: 042ed3e048af08014487d19196984347e3be7d1c\n-- \n2.24.1\n\n"},{"id":"389506","messageId":"20200108214136.GB63040@google.com","threadId":"52589","inReplyTo":"20200108003121.28034-1-kyle@kyleam.com","subject":"Re: [PATCH] submodule add: show 'add --dry-run' stderr when aborting","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-01-08T21:41:36Z","receivedAt":"2020-01-08T21:41:44Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2020.01.07 19:31, Kyle Meyer wrote:\n> Unless --force is specified, 'submodule add' checks if the destination\n> path is ignored by calling 'git add --dry-run --ignore-missing', and,\n> if that call fails, aborts with a custom \"path is ignored\" message (a\n> slight variant of what 'git add' shows).  Aborting early rather than\n> letting the downstream 'git add' call fail is done so that the command\n> exits before cloning into the destination path.  However, in rare\n> cases where the dry-run call fails for a reason other than the path\n> being ignored---for example, due to a preexisting index.lock\n> file---displaying the \"ignored path\" error message hides the real\n> source of the failure.\n> \n> Instead of displaying the tailored \"ignored path\" message, let's\n> report the standard error from the dry run to give the caller more\n> accurate information about failures that are not due to an ignored\n> path.\n> \n> For the ignored path case, this leads to the following change in the\n> error message:\n> \n>   The following [-path is-]{+paths are+} ignored by one of your .gitignore files:\n>   <destination path>\n>   Use -f if you really want to add [-it.-]{+them.+}\n> \n> The new phrasing is a bit awkward, because 'submodule add' is only\n> dealing with one destination path.  Alternatively, we could continue\n> to use the tailored message when the exit code is 1 (the expected\n> status for a failure due to an ignored path) and relay the standard\n> error for all other non-zero exits.  That, however, risks hiding the\n> message of unrelated failures that share an exit code of 1, so it\n> doesn't seem worth doing just to avoid a clunkier, but still clear,\n> error message.\n> \n> Signed-off-by: Kyle Meyer <kyle@kyleam.com>\n> ---\n>  git-submodule.sh           | 14 ++++++++------\n>  t/t7400-submodule-basic.sh | 15 +++++++++++++--\n>  2 files changed, 21 insertions(+), 8 deletions(-)\n> \n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index aaa1809d24..afcb4c0948 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -241,13 +241,15 @@ cmd_add()\n>  \t    die \"$(eval_gettext \"'\\$sm_path' does not have a commit checked out\")\"\n>  \tfi\n>  \n> -\tif test -z \"$force\" &&\n> -\t\t! git add --dry-run --ignore-missing --no-warn-embedded-repo \"$sm_path\" > /dev/null 2>&1\n> +\tif test -z \"$force\"\n>  \tthen\n> -\t\teval_gettextln \"The following path is ignored by one of your .gitignore files:\n> -\\$sm_path\n> -Use -f if you really want to add it.\" >&2\n> -\t\texit 1\n> +\t    dryerr=$(git add --dry-run --ignore-missing --no-warn-embedded-repo \"$sm_path\" 2>&1 >/dev/null)\n> +\t    res=$?\n> +\t    if test $res -ne 0\n> +\t    then\n> +\t\t echo >&2 \"$dryerr\"\n> +\t\t exit $res\n> +\t    fi\n>  \tfi\n>  \n>  \tif test -n \"$custom_name\"\n\nSeems reasonable: we move the dry-run add inside the if block, and\ncapture its stderr, then display the message only if the return code is\nnon-zero.\n\n\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 7f75bb1be6..42a00f95b9 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -156,9 +156,9 @@ test_expect_success 'submodule add to .gitignored path fails' '\n>  \t(\n>  \t\tcd addtest-ignore &&\n>  \t\tcat <<-\\EOF >expect &&\n> -\t\tThe following path is ignored by one of your .gitignore files:\n> +\t\tThe following paths are ignored by one of your .gitignore files:\n>  \t\tsubmod\n> -\t\tUse -f if you really want to add it.\n> +\t\tUse -f if you really want to add them.\n>  \t\tEOF\n>  \t\t# Does not use test_commit due to the ignore\n>  \t\techo \"*\" > .gitignore &&\n> @@ -191,6 +191,17 @@ test_expect_success 'submodule add to reconfigure existing submodule with --forc\n>  \t)\n>  '\n>  \n> +test_expect_success 'submodule add relays add --dry-run stderr' '\n> +\ttest_when_finished \"rm -rf addtest/.git/index.lock\" &&\n> +\t(\n> +\t\tcd addtest &&\n> +\t\t: >.git/index.lock &&\n> +\t\t! git submodule add \"$submodurl\" sub-while-locked 2>output.err &&\n> +\t\ttest_i18ngrep \"^fatal: .*index\\.lock\" output.err &&\n> +\t\ttest_path_is_missing sub-while-locked\n> +\t)\n> +'\n> +\n>  test_expect_success 'submodule add --branch' '\n>  \techo \"refs/heads/initial\" >expect-head &&\n>  \tcat <<-\\EOF >expect-heads &&\n\nI had to look up what \":\" does, but it looks like it's reasonably widely\nused in other tests so that seems fine. However, it looks like you don't\neven need the : command and can just \">.git/index.lock\" by itself (see\nthe \"setup - initial commit\" test case in this file for an example).\n\n> base-commit: 042ed3e048af08014487d19196984347e3be7d1c\n> -- \n> 2.24.1\n> \n\n\nLooks good to me. Thanks for the patch!\n"},{"id":"389516","messageId":"87d0btwdya.fsf@kyleam.com","threadId":"52589","inReplyTo":"20200108214136.GB63040@google.com","subject":"Re: [PATCH] submodule add: show 'add --dry-run' stderr when aborting","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2020-01-09T02:39:25Z","receivedAt":"2020-01-09T02:39:30Z","isPatch":true,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> On 2020.01.07 19:31, Kyle Meyer wrote:\n[...]\n>> +test_expect_success 'submodule add relays add --dry-run stderr' '\n>> +\ttest_when_finished \"rm -rf addtest/.git/index.lock\" &&\n>> +\t(\n>> +\t\tcd addtest &&\n>> +\t\t: >.git/index.lock &&\n[...]\n> I had to look up what \":\" does, but it looks like it's reasonably widely\n> used in other tests so that seems fine. However, it looks like you don't\n> even need the : command and can just \">.git/index.lock\" by itself (see\n> the \"setup - initial commit\" test case in this file for an example).\n\nIndeed.  I spot some recent commits that added new instances of \": >\",\nso I suspect it's not strongly discouraged in this project, but I'm not\nparticularly attached to the unneeded colon :>\n\n> Looks good to me. Thanks for the patch!\n\nThanks for the review!\n"},{"id":"389900","messageId":"nycvar.QRO.7.76.6.2001161711310.46@tvgsbejvaqbjf.bet","threadId":"52589","inReplyTo":"20200108003121.28034-1-kyle@kyleam.com","subject":"Re: [PATCH] submodule add: show 'add --dry-run' stderr when aborting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-16T16:17:33Z","receivedAt":"2020-01-16T16:17:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi (in particular Junio),\n\nOn Tue, 7 Jan 2020, Kyle Meyer wrote:\n\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 7f75bb1be6..42a00f95b9 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -156,9 +156,9 @@ test_expect_success 'submodule add to .gitignored path fails' '\n>  \t(\n>  \t\tcd addtest-ignore &&\n>  \t\tcat <<-\\EOF >expect &&\n> -\t\tThe following path is ignored by one of your .gitignore files:\n> +\t\tThe following paths are ignored by one of your .gitignore files:\n>  \t\tsubmod\n> -\t\tUse -f if you really want to add it.\n> +\t\tUse -f if you really want to add them.\n\nI think this got mis-merged when merging down `km/submodule-add-errmsg`:\nit needs to be prefixed with `hint:` because of `hw/advice-add-nothing`,\ni.e.\n\n-- snipsnap --\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 42a00f95b9d..a6973a3003c 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -158,7 +158,7 @@ test_expect_success 'submodule add to .gitignored path fails' '\n \t\tcat <<-\\EOF >expect &&\n \t\tThe following paths are ignored by one of your .gitignore files:\n \t\tsubmod\n-\t\tUse -f if you really want to add them.\n+\t\thint: Use -f if you really want to add them.\n \t\tEOF\n \t\t# Does not use test_commit due to the ignore\n \t\techo \"*\" > .gitignore &&\n"},{"id":"389923","messageId":"xmqqftgfgngw.fsf@gitster-ct.c.googlers.com","threadId":"52589","inReplyTo":"nycvar.QRO.7.76.6.2001161711310.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] submodule add: show 'add --dry-run' stderr when aborting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-16T20:23:11Z","receivedAt":"2020-01-16T20:23:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> I think this got mis-merged when merging down `km/submodule-add-errmsg`:\n> it needs to be prefixed with `hint:` because of `hw/advice-add-nothing`,\n> i.e.\n>\n> -- snipsnap --\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 42a00f95b9d..a6973a3003c 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -158,7 +158,7 @@ test_expect_success 'submodule add to .gitignored path fails' '\n>  \t\tcat <<-\\EOF >expect &&\n>  \t\tThe following paths are ignored by one of your .gitignore files:\n>  \t\tsubmod\n> -\t\tUse -f if you really want to add them.\n> +\t\thint: Use -f if you really want to add them.\n>  \t\tEOF\n>  \t\t# Does not use test_commit due to the ignore\n>  \t\techo \"*\" > .gitignore &&\n\nThanks, will prepare merge-fix material for this.\n\n\n"}]}