# [PATCH] difftool: add env vars directly in run_file_diff() 3 messages from 2024-05-26 to 2024-06-01. Participants: René Scharfe, Patrick Steinhardt. Thread: https://gitlist.dev/t/61549 ## René Scharfe, 2024-05-26 20:16 Subject: [PATCH] difftool: add env vars directly in run_file_diff() Message-ID: URL: https://gitlist.dev/e/c7c843b9-0ccf-4bcb-a036-d794729a99d6%40web.de ``` Add the environment variables of the child process directly using strvec_push() instead of building an array out of them and then adding that using strvec_pushv(). The new code is shorter and avoids magic array index values and fragile array padding. Signed-off-by: René Scharfe --- builtin/difftool.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/builtin/difftool.c b/builtin/difftool.c index a130faae4f..a1794b7eed 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -674,19 +674,15 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, static int run_file_diff(int prompt, const char *prefix, struct child_process *child) { - const char *env[] = { - "GIT_PAGER=", "GIT_EXTERNAL_DIFF=git-difftool--helper", NULL, - NULL - }; - + strvec_push(&child->env, "GIT_PAGER="); + strvec_push(&child->env, "GIT_EXTERNAL_DIFF=git-difftool--helper"); if (prompt > 0) - env[2] = "GIT_DIFFTOOL_PROMPT=true"; + strvec_push(&child->env, "GIT_DIFFTOOL_PROMPT=true"); else if (!prompt) - env[2] = "GIT_DIFFTOOL_NO_PROMPT=true"; + strvec_push(&child->env, "GIT_DIFFTOOL_NO_PROMPT=true"); child->git_cmd = 1; child->dir = prefix; - strvec_pushv(&child->env, env); return run_command(child); } -- 2.45.1 ``` ## Patrick Steinhardt, 2024-05-29 07:57 Subject: Re: [PATCH] difftool: add env vars directly in run_file_diff() Message-ID: URL: https://gitlist.dev/e/Zlbf1IY4jFlUPD7e%40tanuki In-Reply-To: ``` On Sun, May 26, 2024 at 10:16:50PM +0200, René Scharfe wrote: > Add the environment variables of the child process directly using > strvec_push() instead of building an array out of them and then adding > that using strvec_pushv(). The new code is shorter and avoids magic > array index values and fragile array padding. > > Signed-off-by: René Scharfe I don't know whether this may cause more allocations. But even if it did, it would very much feel like a micro-optimization that is not worth it in the end given that we're about to spawn a new process anyway, which is way more expensive. So favoring readability/maintainability very much feels like the right thing to do here, and the result looks good to me. Thanks! Patrick ``` ## René Scharfe, 2024-06-01 20:23 Subject: Re: [PATCH] difftool: add env vars directly in run_file_diff() Message-ID: URL: https://gitlist.dev/e/f76eaa71-2b04-4d6c-a378-a40010aec22d%40web.de In-Reply-To: ``` Am 29.05.24 um 09:57 schrieb Patrick Steinhardt: > On Sun, May 26, 2024 at 10:16:50PM +0200, René Scharfe wrote: >> Add the environment variables of the child process directly using >> strvec_push() instead of building an array out of them and then adding >> that using strvec_pushv(). The new code is shorter and avoids magic >> array index values and fragile array padding. >> >> Signed-off-by: René Scharfe > > I don't know whether this may cause more allocations. It doesn't -- strvec_pushv() calls strvec_push() in a loop, the patch just unrolls it. René ```