On Thu, Jul 7, 2022 at 9:57 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > > > I may be missing something, but I think this report has nothing to do > > with any recent changes or regressions, but is merely noting a behavior > > change between pre-push and some other hooks that we've had since 1.8.2, > > or since the "pre-push" hook was added in ec55559f937 (push: Add support > > for pre-push hooks, 2013-01-13). > > "behavior change" meaning a regression of a particular hook, or > "behavior difference" between hooks, each of which never changed the > behavior? > > > I tested this with a local v2.30.0, and the behavior was the same. > > I guess you meant the latter. If so, the inconsistency may be > unfortunate, but I agree that it is not cut-and-dried that it is a > good idea to change pre-push to spew its output to standard error > stream. > > > It *is* something we need to be careful of when converting the rest of > > the hooks to the hooks API, i.e. we need tests for how stderr/stdout is > > handled for each one. > > Absolutely. On the one hand, we just had a particularly frustrating regression around hook output direction, and it's likely none of us are interested in repeating such pain anytime soon. On the other hand, I wonder how worried we actually should be about changing the behavior to normalize pre-push and other "oddball" hooks. If the pipe used changes, what changes for the hook author? What about for the person running the hook on their repository? For the hook author, there is no change, in that the hook author cannot control which pipe we decide to send their output to, and should have written their hook with that in mind in the first place. I found one example[1] of a hook which manually sends its only output to stderr, everything else I saw just 'echo'es with abandon. I did notice many hooks relying on colored output, but we've fixed that bug ;) For the hook runner, the main difference has to do with parsing output for scripts. That is, if I'm just looking with my human eyes, I don't care which pipe is used, as long as it comes to the terminal. Of course for scripting that is a different story, and if output I was expecting on a certain pipe suddenly disappears (or vice versa) that is a pain. However, I don't see much evidence that hook output can be used in that way. In fact, I think that is the reason we shunt most hook output to stderr; Git typically says "stdout is what you should parse with your script, and stderr is what humans should look at with their eyeballs." Through the very scientific method of looking at the first 10 pages of GitHub search results[2], I didn't find any evidence that pre-push hooks are being written with the intent for their output to be parsed. That makes sense to me - I'd need to carefully synchronize the output from my pre-push hook *and* the parser in my script, and at that point it makes more sense to just teach the pre-push hook to take whatever action it's trying to communicate to the parser and cut out the interprocess communication, right? Anyway, tl;dr - I think it would actually be a good and reasonable thing to normalize the pre-push hook, either now or at the point where it's converted to hook.c. As for proc-receive, I think that one does something weird - the hook's output is intended to be parsed by Git itself, and isn't shown to the user. proc-receive is in fact such a weird and different hook that it is completely exempt from the migration to hook.c. It does bidirectional communication with the Git parent process and the parent process decides how to act based on that communication, which means that even if run_processes_parallel could support that (it doesn't) it wouldn't make sense to configure more than one proc-receive hook anyways. So proc-receive is intended to continue being an oddball and not use the hook.c library (other than to check for the existence of 0 or 1 proc-receive hooks). - Emily 1: https://github.com/Kaliraj-hesabe/hesabe-reactnative-app/blob/3526fbe3fed8f08dadf10483c1292b8cd8f7d4ed/node_modules/realm/vendor/realm-core/tools/pre-push 2: https://github.com/search?l=&p=9&q=filename%3Apre-push+-filename%3A%2A.sample&type=Code
