{"thread":{"id":"61549","subject":"[PATCH] difftool: add env vars directly in run_file_diff()","startedAt":"2024-05-26T20:16:58Z","lastAt":"2024-06-01T20:23:45Z","messageCount":3,"participants":["René Scharfe","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"495655","messageId":"c7c843b9-0ccf-4bcb-a036-d794729a99d6@web.de","threadId":"61549","inReplyTo":null,"subject":"[PATCH] difftool: add env vars directly in run_file_diff()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-05-26T20:16:50Z","receivedAt":"2024-05-26T20:16:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add the environment variables of the child process directly using\nstrvec_push() instead of building an array out of them and then adding\nthat using strvec_pushv().  The new code is shorter and avoids magic\narray index values and fragile array padding.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/difftool.c | 12 ++++--------\n 1 file changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex a130faae4f..a1794b7eed 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -674,19 +674,15 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n static int run_file_diff(int prompt, const char *prefix,\n \t\t\t struct child_process *child)\n {\n-\tconst char *env[] = {\n-\t\t\"GIT_PAGER=\", \"GIT_EXTERNAL_DIFF=git-difftool--helper\", NULL,\n-\t\tNULL\n-\t};\n-\n+\tstrvec_push(&child->env, \"GIT_PAGER=\");\n+\tstrvec_push(&child->env, \"GIT_EXTERNAL_DIFF=git-difftool--helper\");\n \tif (prompt > 0)\n-\t\tenv[2] = \"GIT_DIFFTOOL_PROMPT=true\";\n+\t\tstrvec_push(&child->env, \"GIT_DIFFTOOL_PROMPT=true\");\n \telse if (!prompt)\n-\t\tenv[2] = \"GIT_DIFFTOOL_NO_PROMPT=true\";\n+\t\tstrvec_push(&child->env, \"GIT_DIFFTOOL_NO_PROMPT=true\");\n\n \tchild->git_cmd = 1;\n \tchild->dir = prefix;\n-\tstrvec_pushv(&child->env, env);\n\n \treturn run_command(child);\n }\n--\n2.45.1\n"},{"id":"495792","messageId":"Zlbf1IY4jFlUPD7e@tanuki","threadId":"61549","inReplyTo":"c7c843b9-0ccf-4bcb-a036-d794729a99d6@web.de","subject":"Re: [PATCH] difftool: add env vars directly in run_file_diff()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-29T07:57:08Z","receivedAt":"2024-05-29T07:57:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, May 26, 2024 at 10:16:50PM +0200, René Scharfe wrote:\n> Add the environment variables of the child process directly using\n> strvec_push() instead of building an array out of them and then adding\n> that using strvec_pushv().  The new code is shorter and avoids magic\n> array index values and fragile array padding.\n> \n> Signed-off-by: René Scharfe <l.s.r@web.de>\n\nI don't know whether this may cause more allocations. But even if it\ndid, it would very much feel like a micro-optimization that is not worth\nit in the end given that we're about to spawn a new process anyway,\nwhich is way more expensive.\n\nSo favoring readability/maintainability very much feels like the right\nthing to do here, and the result looks good to me. Thanks!\n\nPatrick\n"},{"id":"496040","messageId":"f76eaa71-2b04-4d6c-a378-a40010aec22d@web.de","threadId":"61549","inReplyTo":"Zlbf1IY4jFlUPD7e@tanuki","subject":"Re: [PATCH] difftool: add env vars directly in run_file_diff()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-06-01T20:23:39Z","receivedAt":"2024-06-01T20:23:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 29.05.24 um 09:57 schrieb Patrick Steinhardt:\n> On Sun, May 26, 2024 at 10:16:50PM +0200, René Scharfe wrote:\n>> Add the environment variables of the child process directly using\n>> strvec_push() instead of building an array out of them and then adding\n>> that using strvec_pushv().  The new code is shorter and avoids magic\n>> array index values and fragile array padding.\n>>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>\n> I don't know whether this may cause more allocations.\n\nIt doesn't -- strvec_pushv() calls strvec_push() in a loop, the\npatch just unrolls it.\n\nRené\n"}]}