All of lore.kernel.org
 help / color / mirror / Atom feed
From: Adrian Ratiu <adrian.ratiu@collabora.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Patrick Steinhardt <ps@pks.im>,
	Emily Shaffer <emilyshaffer@google.com>,
	Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>,
	Chris Darroch <chrisd@apache.org>,
	"brian m. carlson" <sandals@crustytoothpaste.net>
Subject: Re: [PATCH v2] hook: allow hooks to disable stdout_to_stderr
Date: Wed, 14 Jan 2026 10:59:42 +0200	[thread overview]
Message-ID: <875x94zi0x.fsf@collabora.com> (raw)
In-Reply-To: <878qe0zimo.fsf@gentoo.mail-host-address-is-not-set>

On Wed, 14 Jan 2026, Adrian Ratiu <adrian.ratiu@collabora.com> wrote:
> On Tue, 13 Jan 2026, Jeff King <peff@peff.net> wrote:
>> On Wed, Jan 14, 2026 at 01:45:28AM +0200, Adrian Ratiu wrote:
>>
>>> Changes in v2:
>>> * Extended hook test coverage to detect future regressions (Junio, Patrick)
>>> * Reworded commit message and added explanatory comment (Junio, Patrick)
>>> * Set ungroup = 1 because grouping overrides stdout_to_stderr (Adrian)
>>
>> I have not really been following this topic, but I did read (and
>> reproduce) Kristoffer's earlier report about reading stdin. The fix here
>> was not quite what I expected.
>>
>> In particular...
>>
>>> @@ -93,6 +98,7 @@ struct run_hooks_opt
>>>  #define RUN_HOOKS_OPT_INIT { \
>>>  	.env = STRVEC_INIT, \
>>>  	.args = STRVEC_INIT, \
>>> +	.stdout_to_stderr = 1, \
>>>  }
>>
>> ...I expected to see:
>>
>>   .ungroup = 1, \
>
> Good catch. I actually missed this in v2.
>
> I will drop ungroup from this patch in v3 and add another patch fixing
> Kristoffer's issue (rationale below).
>
>>
>> here. The stdin issue goes back to 857f047e40 (hook: allow overriding
>> the ungroup option, 2025-12-26), where the "ungroup" field was added,
>> and various code paths set it to "1" to match the previous behavior. But
>> any paths that were missed, including run_pre_push_hook(), would see a
>> change of behavior (and in this case, a bug).
>>
>> My reading of 857f047e40 is that it meant to give callers the _option_
>> to switch the ungroup behavior, but not actually change anything. So
>> wouldn't we want to leave the default as it was by initializing it to
>> "1"?
>
> That is correct: my mistake in v2 was assuming Kristoffer and Chris
> reported the same bug, when in fact there are 2 separate bugs requiring
> separate fixes, so I will create 2 separate commits in v3 for each.

Minor correction: I think we need 3 commits for 3 separate bugs we
uncovered (ungroup should have its own commit). :)

Please wait for v3, I will code, test and send it ASAP.

  reply	other threads:[~2026-01-14  9:00 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-01-13 11:56 [PATCH] hook: make stdout_to_stderr optional Adrian Ratiu
2026-01-13 13:36 ` Patrick Steinhardt
2026-01-13 13:55   ` Adrian Ratiu
2026-01-13 14:00 ` Junio C Hamano
2026-01-13 14:06   ` Junio C Hamano
2026-01-13 14:59     ` Adrian Ratiu
2026-01-13 15:22       ` Junio C Hamano
2026-01-13 15:37         ` Adrian Ratiu
2026-01-13 14:11   ` Adrian Ratiu
2026-01-13 23:45 ` [PATCH v2] hook: allow hooks to disable stdout_to_stderr Adrian Ratiu
2026-01-14  3:12   ` Jeff King
2026-01-14  8:46     ` Adrian Ratiu
2026-01-14  8:59       ` Adrian Ratiu [this message]
2026-01-14  9:36       ` Kristoffer Haugsbakk
2026-01-14 17:08       ` Jeff King
2026-01-14 17:19         ` Jeff King
2026-01-14 17:56           ` Adrian Ratiu
2026-01-14  6:13   ` Kristoffer Haugsbakk
2026-01-14 18:57 ` [PATCH v3 0/2] Fix two hook conversion regressions Adrian Ratiu
2026-01-14 18:57   ` [PATCH v3 1/2] hook: allow hooks to disable stdout_to_stderr Adrian Ratiu
2026-01-14 18:57   ` [PATCH v3 2/2] hook: make ungroup opt-out instead of opt-in Adrian Ratiu
2026-01-14 21:27     ` Jeff King
2026-01-14 22:45       ` Adrian Ratiu
2026-01-18  8:44     ` Kristoffer Haugsbakk
2026-01-15 14:15   ` [PATCH v3 0/2] Fix two hook conversion regressions Junio C Hamano
2026-01-15 17:19     ` Adrian Ratiu
2026-01-15 17:33       ` Junio C Hamano
2026-01-15 17:53         ` Adrian Ratiu
2026-01-15 20:27           ` Junio C Hamano
2026-01-15 21:24             ` Adrian Ratiu

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

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=875x94zi0x.fsf@collabora.com \
    --to=adrian.ratiu@collabora.com \
    --cc=chrisd@apache.org \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kristofferhaugsbakk@fastmail.com \
    --cc=peff@peff.net \
    --cc=ps@pks.im \
    --cc=sandals@crustytoothpaste.net \
    /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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.