[PATCH] builtin/receive-pack.c: remove redundant 'if'

[PATCH] builtin/receive-pack.c: remove redundant 'if'

[Taylor Blau]

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(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9aabffa1af..f673e0e76e 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2214,8 +2214,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 			close(err_fd);
 		return "unable to create temporary object directory";
 	}
-	if (tmp_objdir)
-		strvec_pushv(&child.env_array, tmp_objdir_env(tmp_objdir));
+	strvec_pushv(&child.env_array, tmp_objdir_env(tmp_objdir));
 
 	/*
 	 * Normally we just pass the tmp_objdir environment to the child
-- 
2.36.1.94.gb0d54bedca

Re: [PATCH] builtin/receive-pack.c: remove redundant 'if'

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> 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.

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.

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 9aabffa1af..f673e0e76e 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -2214,8 +2214,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
>  			close(err_fd);
>  		return "unable to create temporary object directory";
>  	}
> -	if (tmp_objdir)
> -		strvec_pushv(&child.env_array, tmp_objdir_env(tmp_objdir));
> +	strvec_pushv(&child.env_array, tmp_objdir_env(tmp_objdir));
>  
>  	/*
>  	 * Normally we just pass the tmp_objdir environment to the child

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

Thanks.