threads / patch / 18258

v2connect.c: remove a few globals by using git_config callback data

Subject: [PATCH v2] connect.c: remove a few globals by using git_config callback data

## tl;dr

4 messages between Mar 11, 2009 and Mar 11, 2009. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Erik Faye-Lund· Mar 11, 2009, 02:38 UTC · lore

Since ef90d6d (Provide git_config with a callback-data parameter, 2008-05-14), git_config() takes a callback data pointer that can be used to pass extra parameters to the parsing function. The codepath to parse configuration variables related to git proxy predates this facility and used a pair of file scope static variables instead.

This patch removes the need for these global variables by passing the name of the host we are trying to access as the callback data.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 connect.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

Show changes to connect.c +3 −6

diff --git a/connect.c b/connect.c
index 2f23ab3..0a35cc1 100644
--- a/connect.c
+++ b/connect.c
@@ -373,8 +373,6 @@ static void git_tcp_connect(int fd[2], char *host, int flags)
 
 
 static char *git_proxy_command;
-static const char *rhost_name;
-static int rhost_len;
 
 static int git_proxy_command_options(const char *var, const char *value,
 		void *cb)
@@ -383,6 +381,8 @@ static int git_proxy_command_options(const char *var, const char *value,
 		const char *for_pos;
 		int matchlen = -1;
 		int hostlen;
+		const char *rhost_name = cb;
+		int rhost_len = strlen(rhost_name);
 
 		if (git_proxy_command)
 			return 0;
@@ -426,11 +426,8 @@ static int git_proxy_command_options(const char *var, const char *value,
 
 static int git_use_proxy(const char *host)
 {
-	rhost_name = host;
-	rhost_len = strlen(host);
 	git_proxy_command = getenv("GIT_PROXY_COMMAND");
-	git_config(git_proxy_command_options, NULL);
-	rhost_name = NULL;
+	git_config(git_proxy_command_options, (void*)host);
 	return (git_proxy_command && *git_proxy_command);
 }

-- 
1.6.2.GIT

Johannes Schindelin· Mar 11, 2009, 11:52 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v2] connect.c: remove a few globals by using git_config callback data

Hi,

On Wed, 11 Mar 2009, Erik Faye-Lund wrote:

Show 10 quoted lines

> Since ef90d6d (Provide git_config with a callback-data parameter,
> 2008-05-14), git_config() takes a callback data pointer that can be
> used to pass extra parameters to the parsing function.  The codepath
> to parse configuration variables related to git proxy predates this
> facility and used a pair of file scope static variables instead.
> 
> This patch removes the need for these global variables by passing the
> name of the host we are trying to access as the callback data.
> 
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>

Thanks!

Show 6 quoted lines

> @@ -383,6 +381,8 @@ static int git_proxy_command_options(const char *var, const char *value,
>  		const char *for_pos;
>  		int matchlen = -1;
>  		int hostlen;
> +		const char *rhost_name = cb;
> +		int rhost_len = strlen(rhost_name);

I see that you still calculate the length everytime git_proxy_command_options() is called -- which is for every config variable.

Show 10 quoted lines

> @@ -426,11 +426,8 @@ static int git_proxy_command_options(const char *var, const char *value,
>  
>  static int git_use_proxy(const char *host)
>  {
> -	rhost_name = host;
> -	rhost_len = strlen(host);
>  	git_proxy_command = getenv("GIT_PROXY_COMMAND");
> -	git_config(git_proxy_command_options, NULL);
> -	rhost_name = NULL;
> +	git_config(git_proxy_command_options, (void*)host);

The (void *) should not be needed.

Thanks, Dscho

Erik Faye-Lund· Mar 11, 2009, 13:17 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2] connect.c: remove a few globals by using git_config callback data

Thanks for the feedback.

On Wed, Mar 11, 2009 at 12:52 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 10 quoted lines

>> @@ -383,6 +381,8 @@ static int git_proxy_command_options(const char *var, const char *value,
>>               const char *for_pos;
>>               int matchlen = -1;
>>               int hostlen;
>> +             const char *rhost_name = cb;
>> +             int rhost_len = strlen(rhost_name);
>
> I see that you still calculate the length everytime
> git_proxy_command_options() is called -- which is for every config
> variable.

Actually, the length-calculation is now moved inside the check for "core.gitproxy", so it should only trigger for each config variable that is "core.gitproxy". Hopefully, this should usually be just once ;)

This change was suggested as a compromise by Junio.

>
>> +     git_config(git_proxy_command_options, (void*)host);
>
> The (void *) should not be needed.

Actually, it is - it casts away the const-ness of the string. Removing the cast generates a warning on my installation of GCC. Perhaps I should have made that clearer by casting to "char*" instead of "void*"?

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Johannes Schindelin· Mar 11, 2009, 13:28 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v2] connect.c: remove a few globals by using git_config callback data

Hi,

On Wed, 11 Mar 2009, Erik Faye-Lund wrote:

Show 19 quoted lines

> On Wed, Mar 11, 2009 at 12:52 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >> @@ -383,6 +381,8 @@ static int git_proxy_command_options(const char *var, const char *value,
> >>               const char *for_pos;
> >>               int matchlen = -1;
> >>               int hostlen;
> >> +             const char *rhost_name = cb;
> >> +             int rhost_len = strlen(rhost_name);
> >
> > I see that you still calculate the length everytime
> > git_proxy_command_options() is called -- which is for every config
> > variable.
> 
> Actually, the length-calculation is now moved inside the check for
> "core.gitproxy", so it should only trigger for each config variable
> that is "core.gitproxy". Hopefully, this should usually be just once
> ;)
> 
> This change was suggested as a compromise by Junio.

Ah, okay, I am lacking time to read all emails today, so I missed that one.

Show 8 quoted lines

> >> +     git_config(git_proxy_command_options, (void*)host);
> >
> > The (void *) should not be needed.
> 
> Actually, it is - it casts away the const-ness of the string. Removing
> the cast generates a warning on my installation of GCC. Perhaps I
> should have made that clearer by casting to "char*" instead of
> "void*"?

Oh, that's why... Thanks for the clarification! Now only a space between the void and the star, and I am happy!

Ciao, Dscho