From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, "Damien Marié" <damien@dam.io>
Subject: Re: [PATCH v3] run-command: add hint when a hook is ignored
Date: Wed, 3 Jan 2018 03:31:46 -0500 [thread overview]
Message-ID: <20180103083145.GA7049@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqqmv4ymc7w.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 11, 2017 at 03:26:43PM +0900, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> 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
next prev parent reply other threads:[~2018-01-03 8:31 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-03 10:59 [PATCH] run-command.c: add hint when hook is not executable Damien
2017-10-04 4:40 ` Junio C Hamano
2017-10-05 20:48 ` Damien
2017-10-05 20:53 ` [PATCH v2] run-command: add hint when a hook is ignored Damien Marié
2017-10-06 4:52 ` Junio C Hamano
2017-10-06 5:25 ` Junio C Hamano
2017-10-06 5:43 ` Junio C Hamano
2017-10-06 5:53 ` Junio C Hamano
2017-10-06 8:04 ` Damien
2017-10-06 8:07 ` [PATCH v3] " Damien Marié
2017-10-10 4:21 ` Junio C Hamano
2017-10-11 6:26 ` Junio C Hamano
2018-01-03 8:31 ` Jeff King [this message]
2018-01-05 19:36 ` Junio C Hamano
2018-03-03 7:16 ` Jeff King
2018-03-05 21:19 ` 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=20180103083145.GA7049@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=damien@dam.io \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.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).