git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
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
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


  reply index

Thread overview: 15+ 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 publically 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

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox