On Mon, Nov 22 2021, Enzo Matsumiya wrote:
> On 11/21, Junio C Hamano wrote:
>>It is rather common for us to reuse "struct child_process" in a code
>>path, e.g. builtin/worktree.c::add_worktree() prepares a single
>>instance of such a struct, sets cp.git_cmd to true, and runs either
>>"update-ref" or "symbolic-ref" to update "HEAD". After successful
>>invocation of such a git subcommand, it then runs "git reset --hard",
>>with this piece of code:
>
> Do you agree that at least NULLing .argv and .env could be part of
> child_process_clear()?
>
> diff --git a/run-command.c b/run-command.c
> index a7bf81025afb..3839a26eff11 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -18,8 +18,9 @@ void child_process_init(struct child_process *child)
> void child_process_clear(struct child_process *child)
> {
> strvec_clear(&child->args);
> + child->argv = NULL;
> strvec_clear(&child->env_array);
> + child->env = NULL;
> }
>
> With this change on main, all tests are successful.
That's what your patch that calls child_process_init() is trying to do,
but it fails tests on "master". The "child" pointer here is valid, so
what's going on with the memcpy() not working, but these two assignments
doing the job?
I haven't looked into why, but I'd think whatever the difference is
should be moved into making child_process_init() handle that, and
continuing to call that in child_process_clear(), if we go for the
approach in your patch.
I think an alternate direction of simply getting rid of "argv" is better
in this case, and I've just submitted a topic to do that:
https://lore.kernel.org/git/cover-0.5-00000000000-20211122T153605Z-avarab@xxxxxxxxx/
It includes the test you noted to make the pager crash.
It still leave us with this oddity:
$ ~/g/git/git -c pager.show=INVALID_PAGER show HEAD
error: cannot run INVALID_PAGER: No such file or directory
error: cannot run INVALID_PAGER: No such file or directory
But that was the case before that topic (if we hadn't
crashed/segfaulted), and with your proposed change here.
