patchbuiltin/receive-pack.c: remove redundant 'if'

In c7c4bdeccf (run-command API: remove "env" member, always use "env_array", 2021-11-25), there was a push to replace

    cld.env = env->v;

with

    strvec_pushv(&cld.env_array, env->v);

The conversion in c7c4bdeccf was mostly plug-and-play, with the snag that some instances of strvec_pushv() became guarded with a NULL check to ensure that the second argument was non-NULL.

This conversion was slightly over-eager to add a conditional in builtin/receive-pack.c::unpack(), since we know at the point that we add the result of `tmp_objdir_env()` into the child process's environment, that `tmp_objdir` is non-NULL.

This follows from the conditional just before our strvec_pushv() call (which returns from the function if `tmp_objdir` was NULL), as well as the call to tmp_objdir_add_as_alternate() just below, which relies on its argument (`tmp_objdir`) being non-NULL.

In the meantime, this extra conditional isn't hurting anything. But it is redundant and thus unnecessarily confusing. So let's remove it.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/receive-pack.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Taylor Blau <me@ttaylorr.com> writes:

Thanks. I am not sure about the reference to plug-and-play but most of the changes in the commit were mechanical conversion akin to

	@@
	struct child_process C;
	expression V;
	@@
	-C.env = V;
	+strvec_pushv(&C.env_array, V);

but a few have rewritten them to

	@@
	-C.env = V;
	+if (V) strvec_pushv(&C.env_array, V);

And this hunk is one of them. If we know V is never NULL, the check becomes redundant, and we can tell tmp_objdir is not NULL at this point in the control flow. If C.env (or C.env_array) weren't empty by the time the control reaches at this point, however, this rewrite changes behaviour X-<.

Another one in connected.c looks like this:

        diff --git a/connected.c b/connected.c
        index 35bd4a2638..ed3025e7a2 100644
        --- a/connected.c
        +++ b/connected.c
        @@ -109,7 +109,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
                                     _("Checking connectivity"));

                rev_list.git_cmd = 1;
        -	rev_list.env = opt->env;
        +	if (opt->env)
        +		strvec_pushv(&rev_list.env_array, opt->env);
                rev_list.in = -1;
                rev_list.no_stdout = 1;
                if (opt->err_fd)

If opt->env can be NULL, the original code would have just cleared existing rev_list.env but new code instead keeps whatever is in rev_list.env, which technically speaking changes the behaviour but we are saved by the fact that rev_list.env hasn't been touched at this point in the control flow since it was initialized, which makes it a correct conversion.

There are few others in editor.c::launch_specified_editor(), run-command.c::run_command_v_opt_cd_env_tr2() and run-command.c::run_hook_ve(), but the story is identical in these places.

So, in short, looks good, well spotted, and will apply.

Thanks.