threads / patch / 61549

patchdifftool: add env vars directly in run_file_diff()

Subject: [PATCH] difftool: add env vars directly in run_file_diff()

## tl;dr

3 messages between May 26, 2024 and Jun 1, 2024. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

René Scharfe· May 26, 2024, 20:16 UTC · lore

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 <l.s.r@web.de>
---
 builtin/difftool.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Show changes to builtin/difftool.c +4 −9

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· May 29, 2024, 07:57 UTC · re: René Scharfe · lore

Re: [PATCH] difftool: add env vars directly in run_file_diff()

On Sun, May 26, 2024 at 10:16:50PM +0200, René Scharfe wrote:

Show 6 quoted lines

> 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 <l.s.r@web.de>

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· Jun 1, 2024, 20:23 UTC · re: Patrick Steinhardt · lore

Re: [PATCH] difftool: add env vars directly in run_file_diff()

Am 29.05.24 um 09:57 schrieb Patrick Steinhardt:

Show 9 quoted lines

> 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 <l.s.r@web.de>
>
> 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é