| Message ID | patch-5.6-98c26c9917b-20220421T122108Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hook API: connect hooks to the TTY again, fixes a v2.36.0 regression | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: [jc: I stopped reviewing at 4/6 in the last batch, thought that it was enough for a reroll and shifted my attention to address other regressions. Let me come back to this topic to finish commenting before a reroll comes.] > Amend code added in 96e7225b310 (hook: add 'run' subcommand, > 2021-12-22) top stop setting these two flags. We use the "top stop"? -ECANNOTPARSE. > run_process_parallel() API added in c553c72eed6 (run-command: add an > asynchronous parallel child processor, 2015-12-15), which always sets > these in pp_start_one() (in addition to setting .err = -1). > > Note that an assert() to check that these values are already what > we're setting them to here would fail. That's because in > pp_start_one() we'll set these after calling this "get_next_task" > callback (which we call pick_next_hook()). But the only case where we > weren't setting these just after returning from this function was if > we took the "return 0" path here, in which case we wouldn't have set > these. > > So while this code wasn't wrong, it was entirely redundant. The > run_process_parallel() also can't work with a generic "struct > child_process", it needs one that's behaving in a way that it expects > when it comes to stderr/stdout. So we shouldn't be changing these > values, or in this case keeping around code that gives the impression > that doing in the general case is OK. OK. As long as we set these two fields correctly (i.e. the hooks do not read from the standard input, and stdout_to_stderr is in effect (i.e. dup2(2, 1) is done), by the time we pass this into run_command() API, this step would be a benign no-op. Good. Not that it is something I would expect to see in a rather urgent post-release regression fix, though. > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > hook.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/hook.c b/hook.c > index eadb2d58a7b..68ee4030551 100644 > --- a/hook.c > +++ b/hook.c > @@ -53,9 +53,7 @@ static int pick_next_hook(struct child_process *cp, > if (!hook_path) > return 0; > > - cp->no_stdin = 1; > strvec_pushv(&cp->env_array, hook_cb->options->env.v); > - cp->stdout_to_stderr = 1; > cp->trace2_hook_name = hook_cb->hook_name; > cp->dir = hook_cb->options->dir;
diff --git a/hook.c b/hook.c index eadb2d58a7b..68ee4030551 100644 --- a/hook.c +++ b/hook.c @@ -53,9 +53,7 @@ static int pick_next_hook(struct child_process *cp, if (!hook_path) return 0; - cp->no_stdin = 1; strvec_pushv(&cp->env_array, hook_cb->options->env.v); - cp->stdout_to_stderr = 1; cp->trace2_hook_name = hook_cb->hook_name; cp->dir = hook_cb->options->dir;
Amend code added in 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) top stop setting these two flags. We use the run_process_parallel() API added in c553c72eed6 (run-command: add an asynchronous parallel child processor, 2015-12-15), which always sets these in pp_start_one() (in addition to setting .err = -1). Note that an assert() to check that these values are already what we're setting them to here would fail. That's because in pp_start_one() we'll set these after calling this "get_next_task" callback (which we call pick_next_hook()). But the only case where we weren't setting these just after returning from this function was if we took the "return 0" path here, in which case we wouldn't have set these. So while this code wasn't wrong, it was entirely redundant. The run_process_parallel() also can't work with a generic "struct child_process", it needs one that's behaving in a way that it expects when it comes to stderr/stdout. So we shouldn't be changing these values, or in this case keeping around code that gives the impression that doing in the general case is OK. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- hook.c | 2 -- 1 file changed, 2 deletions(-)