Re: [PATCH v3] run-command: add hint when a hook is ignored

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 5, 2018, 19:36 UTC
Message-ID: <xmqqk1wwcd2w.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20180103083145.GA7049@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 16 quoted lines

> 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.

True. The user could tell the server operator to rename them or remove them because they are not doing anything useful, but then as long as everybody knows they are not doing anything, it is OK to leave that sleeping dog lie, as they are not doing anything harmful anyway.

That brings us one step further back to question if the hints are useful in the first place, though ;-).

>   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.

I do not think having a non-executable file whose name happens to be the same as a hook is an error in the first place [*1*], so I do not think it is unreasonable for --quiet to squelch.

	side note [*1*]: we need to make sure that it is clearly
	documented that such a file is not a hook, of course.

>   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.

This probably is much deeper issue and templates/hooks--* is merely a tip of iceberg, I suspect. In general build artifacts from different versions of Git in the same build area could interfere the tests in other ways (e.g. a test for "git help" output could try to grab a deprecated command left in bin-wrappers/ from previous build of an ancient version), and could potentially interfere things other than the tests (e.g. the 'install' target of the Makefile may be written loosely to install everything in a directory).

In general, I am a bit pessimistic and suspect that the best we could say is "if you want to absolutely avoid such interference, 'make distclean' before switching to test another revision".

>   4. Should our build system be more clever about dropping non-existent
>      files from templates/blt?

It would be a reasonable mitigation that is specific to templates/, I suspect.