On Wed, Oct 11, 2017 at 03:26:43PM +0900, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
> > Quite honestly, I do not particulary think this is confusing, and I
> > expect that this change will irritate many people by forcing them to
> > either set the advise config or move the ones that they deliberately
> > left unexecutable by renaming them by adding ".disabled" at the end.
> >
> > But these remedies are easy enough, so let's see how well it works
> > by merging it to 'next' and cooking it there for a while.
>
> Well, it turns out that I am among those who are irritated, as all
> the repositories I work with were rather old, dating back to 2005,
> back when it was a norm to have these sample files installed without
> executable bit, to make it easy for those who choose to use them
> as-is to enable them by flipping the executable bit.
> [...]
> Anyway, I am not merging this topic to the upcoming release, so
> hopefully we'll hear from others who try 'next'.
This bit me today in a funny way: t5523 started failing.
The problem is that I was bisecting an unrelated change in old code, and
I built a commit which predates f98f8cbac0 (Ship sample hooks with .sample
suffix, 2008-06-24). That wrote a bare "update" file into templates/blt
(without the ".sample" suffix). After that, the cruft is forever in my
build directory until I "git clean -x".
The t5523 script tries to run "git push --quiet" and make sure that it
produces no output, but with this setup it produces a round of "hint"
lines (actually, they come from receive-pack on the remote end but still
end up on stderr).
So that raises a few interesting questions to me:
1. Should receive-pack generate these hints? They get propagated by
the client, but there's a reasonable chance that the user can't
actually fix the situation.
2. Should these hints be suppressed with --quiet? I can see an
argument that "--quiet" only applies to non-errors. And while these
are not fatal errors, they're outside the realm of the usual
chattiness.
3. Should our tests be more careful about not looking at the
template hooks? I think test_create_repo already disables the hooks
directory manually, but many repos will be created by "git clone"
during the tests.
4. Should our build system be more clever about dropping non-existent
files from templates/blt?
I started down the road of saying "--quiet should disable all advice",
and that patch is below. But I'm having second thoughts on it. It fixes
the case in receive-pack, but what should "commit --quiet" do? It's
documented only to suppress the status output after commit. Should it
cover this case? For that matter, the "quiet" protocol extension which
is passed to receive-pack is generally used only for progress reporting,
and is sent automatically when stderr isn't a tty (which is why the
tests below must go through some contortions with test_terminal). I'm
not sure if it should actually apply to advice.
---
advice.c | 8 ++++++++
advice.h | 6 ++++++
builtin/receive-pack.c | 4 +++-
t/t7520-ignored-hook-warning.sh | 15 +++++++++++++++
4 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/advice.c b/advice.c
index 406efc183b..f754af8abe 100644
--- a/advice.c
+++ b/advice.c
@@ -137,3 +137,11 @@ void detach_advice(const char *new_name)
fprintf(stderr, fmt, new_name);
}
+
+void disable_advice(void)
+{
+ size_t i;
+
+ for (i = 0; i < ARRAY_SIZE(advice_config); i++)
+ *advice_config[i].preference = 0;
+}
diff --git a/advice.h b/advice.h
index 70568fa792..4895554ef3 100644
--- a/advice.h
+++ b/advice.h
@@ -30,4 +30,10 @@ extern void NORETURN die_resolve_conflict(const char *me);
void NORETURN die_conclude_merge(void);
void detach_advice(const char *new_name);
+/*
+ * Turn off all advice flags; this can be used to centrally enforce a --quiet
+ * option.
+ */
+void disable_advice(void);
+
#endif /* ADVICE_H */
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b7ce7c7f52..f257c16776 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1601,8 +1601,10 @@ static struct command *read_head_info(struct oid_array *shallow)
report_status = 1;
if (parse_feature_request(feature_list, "side-band-64k"))
use_sideband = LARGE_PACKET_MAX;
- if (parse_feature_request(feature_list, "quiet"))
+ if (parse_feature_request(feature_list, "quiet")) {
quiet = 1;
+ disable_advice();
+ }
if (advertise_atomic_push
&& parse_feature_request(feature_list, "atomic"))
use_atomic = 1;
diff --git a/t/t7520-ignored-hook-warning.sh b/t/t7520-ignored-hook-warning.sh
index 634fb7f23a..1d27d3e3f0 100755
--- a/t/t7520-ignored-hook-warning.sh
+++ b/t/t7520-ignored-hook-warning.sh
@@ -3,6 +3,7 @@
test_description='ignored hook warning'
. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
test_expect_success setup '
hookdir="$(git rev-parse --git-dir)/hooks" &&
@@ -38,4 +39,18 @@ test_expect_success 'no warning if unset advice.ignoredHook and hook removed' '
test_i18ngrep ! -e "hook was ignored" message
'
+test_expect_success TTY,POSIXPERM 'push --quiet silences remote hook warnings' '
+ git init --bare dst.git &&
+ echo "exit 0" >dst.git/hooks/update &&
+ chmod -x dst.git/hooks/update &&
+
+ git commit --allow-empty -m one &&
+ test_terminal git push dst.git HEAD 2>message &&
+ test_i18ngrep -e "hook was ignored" message &&
+
+ git commit --allow-empty -m two &&
+ test_terminal git push --quiet dst.git HEAD 2>message &&
+ test_i18ngrep ! -e "hook was ignored" message
+'
+
test_done
--
2.16.0.rc0.384.gc477e89267
