Re: pre-push hooks and stdout regression

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 3023 bytes --]

On 2026-01-12 at 23:21:42, Chris Darroch wrote:
> Hello --
> 
>    I'm one of the current maintainers of the Git LFS project, and we
> happened to notice that a recent change in Git's "master" branch has
> introduced a regression in our test suite.
> 
>    Specifically, with commit 3e2836a742d8b2b2da25ca06e9d0ac3a539bd966
> ("transport: convert pre-push to hook API") from the "ar/run-command-hook"
> merged last week, it appears that when a pre-push hook such as our
> git-lfs-pre-push program runs, messages it writes to its standard output
> are now delivered to the user's standard error stream instead of
> their standard output stream.

CCing the author and submitter.

>    I suspect this is because the pick_next_hook() function in hook.c
> sets the stdout_to_stderr flag for its "cb" child_process argument,
> and that function is now used to run the pre-push hook.

That's likely the case.  96e7225b31 ("hook: add 'run' subcommand",
2021-12-22), which introduced that code, didn't provide an explanation
for that decision.  I think that's required for hooks such as
pre-receive that send data over the sideband, and it may be that the
code was simply reused.  Of course, I could be mistaken.

>    However, I think there remains the larger concern that users who
> depend on the existing Git pre-push behaviour in some way may also
> encounter regressions, perhaps because they expect (as our test suite
> does) to see certain messages either output or not output to stderr
> during a Git push operation.

I suspect that we're also going to see similar regressions in other
software.  pre-push hooks are really popular, as well as pre-commit
hooks, and people are going to expect both standard output and standard
error to be connected to the same place they were before.

A quick check of pre-push hook frameworks on GitHub indicates that a
substantial number of them print to standard output, even for error
messages.  I expect that we will actually see quite a bit of breakage in
this case, especially from automated tooling and Git graphical
frontends, which may expect data in a certain place.  I don't have any
proof of this, however, but it does seem like the kind of thing that
people will have come to rely on.

I'd recommend that we avoid using `stdout_to_stderr` for hooks unless we
did originally or really need to (e.g., for things that might go over
the sideband).

>    Please do let me know your thoughts on this subject!  If the
> consensus is that the new behaviour is correct, we'll adjust our test
> suite to match it, but I'll wait to hear the outcome of any discussion
> before making that change.

I will just state for the benefit of the list that Chris and I are
colleagues and we discussed this at work, so I asked him to CC me on
this issue since I was curious.  My opinions here, as with everything
originating from this email address, are mine and not my employer's.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]