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

[Junio C Hamano <gitster@xxxxxxxxx>]

Taylor Blau <me@xxxxxxxxxxxx> 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.