{"thread":{"id":"57894","subject":"[PATCH] builtin/receive-pack.c: remove redundant 'if'","startedAt":"2022-05-18T20:26:14Z","lastAt":"2022-05-18T21:16:34Z","messageCount":2,"participants":["Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"455424","messageId":"d22f2ca975778d594449857d64be8cd8c0d4a327.1652905549.git.me@ttaylorr.com","threadId":"57894","inReplyTo":null,"subject":"[PATCH] builtin/receive-pack.c: remove redundant 'if'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-18T20:26:02Z","receivedAt":"2022-05-18T20:26:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In c7c4bdeccf (run-command API: remove \"env\" member, always use\n\"env_array\", 2021-11-25), there was a push to replace\n\n    cld.env = env->v;\n\nwith\n\n    strvec_pushv(&cld.env_array, env->v);\n\nThe conversion in c7c4bdeccf was mostly plug-and-play, with the snag\nthat some instances of strvec_pushv() became guarded with a NULL check\nto ensure that the second argument was non-NULL.\n\nThis conversion was slightly over-eager to add a conditional in\nbuiltin/receive-pack.c::unpack(), since we know at the point that we\nadd the result of `tmp_objdir_env()` into the child process's\nenvironment, that `tmp_objdir` is non-NULL.\n\nThis follows from the conditional just before our strvec_pushv() call\n(which returns from the function if `tmp_objdir` was NULL), as well as\nthe call to tmp_objdir_add_as_alternate() just below, which relies on\nits argument (`tmp_objdir`) being non-NULL.\n\nIn the meantime, this extra conditional isn't hurting anything. But it\nis redundant and thus unnecessarily confusing. So let's remove it.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/receive-pack.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9aabffa1af..f673e0e76e 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2214,8 +2214,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\t\tclose(err_fd);\n \t\treturn \"unable to create temporary object directory\";\n \t}\n-\tif (tmp_objdir)\n-\t\tstrvec_pushv(&child.env_array, tmp_objdir_env(tmp_objdir));\n+\tstrvec_pushv(&child.env_array, tmp_objdir_env(tmp_objdir));\n \n \t/*\n \t * Normally we just pass the tmp_objdir environment to the child\n-- \n2.36.1.94.gb0d54bedca\n"},{"id":"455427","messageId":"xmqqo7zutuck.fsf@gitster.g","threadId":"57894","inReplyTo":"d22f2ca975778d594449857d64be8cd8c0d4a327.1652905549.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/receive-pack.c: remove redundant 'if'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-18T21:16:11Z","receivedAt":"2022-05-18T21:16:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> The conversion in c7c4bdeccf was mostly plug-and-play, with the snag\n> that some instances of strvec_pushv() became guarded with a NULL check\n> to ensure that the second argument was non-NULL.\n\nThanks.  I am not sure about the reference to plug-and-play but most\nof the changes in the commit were mechanical conversion akin to\n\n\t@@\n\tstruct child_process C;\n\texpression V;\n\t@@\n\t-C.env = V;\n\t+strvec_pushv(&C.env_array, V);\n\nbut a few have rewritten them to\n\n\t@@\n\t-C.env = V;\n\t+if (V) strvec_pushv(&C.env_array, V);\n\nAnd this hunk is one of them.  If we know V is never NULL, the check\nbecomes redundant, and we can tell tmp_objdir is not NULL at this\npoint in the control flow.  If C.env (or C.env_array) weren't empty\nby the time the control reaches at this point, however, this rewrite\nchanges behaviour X-<.\n\nAnother one in connected.c looks like this:\n\n        diff --git a/connected.c b/connected.c\n        index 35bd4a2638..ed3025e7a2 100644\n        --- a/connected.c\n        +++ b/connected.c\n        @@ -109,7 +109,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n                                     _(\"Checking connectivity\"));\n\n                rev_list.git_cmd = 1;\n        -\trev_list.env = opt->env;\n        +\tif (opt->env)\n        +\t\tstrvec_pushv(&rev_list.env_array, opt->env);\n                rev_list.in = -1;\n                rev_list.no_stdout = 1;\n                if (opt->err_fd)\n\nIf opt->env can be NULL, the original code would have just cleared\nexisting rev_list.env but new code instead keeps whatever is in\nrev_list.env, which technically speaking changes the behaviour but\nwe are saved by the fact that rev_list.env hasn't been touched at\nthis point in the control flow since it was initialized, which makes\nit a correct conversion.\n\nThere are few others in editor.c::launch_specified_editor(),\nrun-command.c::run_command_v_opt_cd_env_tr2() and\nrun-command.c::run_hook_ve(), but the story is identical in these\nplaces.\n\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 9aabffa1af..f673e0e76e 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -2214,8 +2214,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n>  \t\t\tclose(err_fd);\n>  \t\treturn \"unable to create temporary object directory\";\n>  \t}\n> -\tif (tmp_objdir)\n> -\t\tstrvec_pushv(&child.env_array, tmp_objdir_env(tmp_objdir));\n> +\tstrvec_pushv(&child.env_array, tmp_objdir_env(tmp_objdir));\n>  \n>  \t/*\n>  \t * Normally we just pass the tmp_objdir environment to the child\n\nSo, in short, looks good, well spotted, and will apply.\n\nThanks.\n"}]}