[PATCH] Removed unnecessary use of global variables.

[PATCH] Removed unnecessary use of global variables.

[Erik Faye-Lund]

git_config() now takes a third data-parameter that is passed back
to the callback-function. At the time this code was written, that
parameter did not exist, so a somewhat nasty (but by all means
correct) use of global variables was introduced. In commit
ef90d6d4208a5130185b04f06e5f90a5f9959fe3 Johannes Schindelin
<Johannes.Schindelin@gmx.de> introduced a parameter for similar
purposes.

I've changed the code to utilize this parameter to pass the
string. In addition, I've made the function calculate the string
length on usage instead, to reduce the parameters needed to what
the callback-interface supplies.

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

diff --git a/connect.c b/connect.c
index 2f23ab3..98fbaea 100644
--- a/connect.c
+++ b/connect.c
@@ -371,14 +371,13 @@ static void git_tcp_connect(int fd[2], char *host, int flags)
 	fd[1] = dup(sockfd);
 }
 
-
 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)
+		void *data)
 {
+	const char *rhost_name = data;
+	const size_t rhost_len = strlen(rhost_name);
+
 	if (!strcmp(var, "core.gitproxy")) {
 		const char *for_pos;
 		int matchlen = -1;
@@ -421,16 +420,13 @@ static int git_proxy_command_options(const char *var, const char *value,
 		return 0;
 	}
 
-	return git_default_config(var, value, cb);
+	return git_default_config(var, value, data);
 }
 
 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

[PATCH] Removed unnecessary use of global variables.

[Erik Faye-Lund]

git_config() now takes a third data-parameter that is passed back
to the callback-function. At the time this code was written, that
parameter did not exist, so a somewhat nasty (but by all means
correct) use of global variables was introduced. In commit
ef90d6d4208a5130185b04f06e5f90a5f9959fe3 Johannes Schindelin
<Johannes.Schindelin@gmx.de> introduced a parameter for similar
purposes.

I've changed the code to utilize this parameter to pass the
string. In addition, I've made the function calculate the string
length on usage instead, to reduce the parameters needed to what
the callback-interface supplies.

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

diff --git a/connect.c b/connect.c
index 2f23ab3..98fbaea 100644
--- a/connect.c
+++ b/connect.c
@@ -371,14 +371,13 @@ static void git_tcp_connect(int fd[2], char *host, int flags)
 	fd[1] = dup(sockfd);
 }
 
-
 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)
+		void *data)
 {
+	const char *rhost_name = data;
+	const size_t rhost_len = strlen(rhost_name);
+
 	if (!strcmp(var, "core.gitproxy")) {
 		const char *for_pos;
 		int matchlen = -1;
@@ -421,16 +420,13 @@ static int git_proxy_command_options(const char *var, const char *value,
 		return 0;
 	}
 
-	return git_default_config(var, value, cb);
+	return git_default_config(var, value, data);
 }
 
 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

Re: Removed unnecessary use of global variables.

[Erik Faye-Lund]

Sorry about the whole double-post thing, this whole submitting patches
over email thing is new to me, and I'm making too many mistakes right
now. Hopefully, I'll improve in the future ;)

On Mar 11, 1:09 am, Erik Faye-Lund <kusmab...@gmail.com> wrote:
> git_config() now takes a third data-parameter that is passed back
> to the callback-function. At the time this code was written, that
> parameter did not exist, so a somewhat nasty (but by all means
> correct) use of global variables was introduced. In commit
> ef90d6d4208a5130185b04f06e5f90a5f9959fe3 Johannes Schindelin
> <Johannes.Schinde...@gmx.de> introduced a parameter for similar
> purposes.
>
> I've changed the code to utilize this parameter to pass the
> string. In addition, I've made the function calculate the string
> length on usage instead, to reduce the parameters needed to what
> the callback-interface supplies.
>
> Signed-off-by: Erik Faye-Lund <kusmab...@gmail.com>
> ---
>  connect.c |   16 ++++++----------
>  1 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/connect.c b/connect.c
> index 2f23ab3..98fbaea 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -371,14 +371,13 @@ static void git_tcp_connect(int fd[2], char *host, int flags)
>         fd[1] = dup(sockfd);
>  }
>
> -
>  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)
> +               void *data)
>  {
> +       const char *rhost_name = data;
> +       const size_t rhost_len = strlen(rhost_name);
> +
>         if (!strcmp(var, "core.gitproxy")) {
>                 const char *for_pos;
>                 int matchlen = -1;
> @@ -421,16 +420,13 @@ static int git_proxy_command_options(const char *var, const char *value,
>                 return 0;
>         }
>
> -       return git_default_config(var, value, cb);
> +       return git_default_config(var, value, data);
>  }
>
>  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
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Removed unnecessary use of global variables.

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> Subject: Re: [PATCH] Removed unnecessary use of global variables.

Please review "git log" and "git shortlog" output for the past dozen or so
commits to learn the style used in the project.

The first paragraph is a single line that goes to "Subject: ".  It
typically:

   - mentions what area, file, or function is affected;

   - a colon;

   - command the person who applies the patch to do something, iow,
     phrased in imperative mood; and

   - lacks the terminating full-stop.

   That is:

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

> git_config() now takes a third data-parameter that is passed back
> to the callback-function. At the time this code was written, that
> parameter did not exist, so a somewhat nasty (but by all means
> correct) use of global variables was introduced. In commit
> ef90d6d4208a5130185b04f06e5f90a5f9959fe3 Johannes Schindelin
> <Johannes.Schindelin@gmx.de> introduced a parameter for similar
> purposes.

Perfect, even though I would have abbreviated the commit object name and
said:

    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.

> I've changed the code to utilize this parameter to pass the
> string. In addition, I've made the function calculate the string
> length on usage instead, to reduce the parameters needed to what
> the callback-interface supplies.

We tend to avoid saying "I did this", i.e.

    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 |   16 ++++++----------
>  1 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/connect.c b/connect.c
> index 2f23ab3..98fbaea 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -371,14 +371,13 @@ static void git_tcp_connect(int fd[2], char *host, int flags)
>  	fd[1] = dup(sockfd);
>  }
>  
> -
>  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)
> +		void *data)
>  {
> +	const char *rhost_name = data;
> +	const size_t rhost_len = strlen(rhost_name);
> +

This is bad for two and half reasons.

 - The original said "cb" and we use that to denote "callback"
   everywhere.  Renaming it to "data" adds noise to the patch without
   adding any extra value.

 - The original used "int" but you made it to an unsigned "size_t"; a
   type-change like this could change the semantics.

   I've read the codepath and it does not seem to introduce a bug, but if
   you are changing it to size_t, then you should change the other two
   variables (host_len and match_len) that are compared with (and are
   involved in subtraction with) rhost_len to the same type at the same
   time to avoid confusion and potential for a bug.

 - Parser functions given to git_config(), such as this one, are called
   for each and every configuration datum encountered in the config files
   (/etc/gitconfig, $HOME/.gitconfig, and .git/config).  Because you
   decided not to precompute rhost_len at the calling site, you are
   recomputing it every time this function is called.  Worse yet, you
   compute it even before deciding if the call is about "core.gitproxy"
   variable.

   Moving these two variables down one scope would be a reasonable
   compromise.  You could introduce a structure with two members (name,
   len) and pass it as a call-back data, but for this particular case,
   counting the length of the name every time you see "core.gitproxy" in
   the configuration file is acceptable.

Thanks.

Re: [PATCH] Removed unnecessary use of global variables.

[Johannes Schindelin]

Hi,

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

> git_config() now takes a third data-parameter that is passed back
> to the callback-function. At the time this code was written, that
> parameter did not exist, so a somewhat nasty (but by all means
> correct) use of global variables was introduced. In commit
> ef90d6d4208a5130185b04f06e5f90a5f9959fe3 Johannes Schindelin
> <Johannes.Schindelin@gmx.de> introduced a parameter for similar
> purposes.

We tend to quote commits in this form: ef90d6d(Provide git_config with a 
callback-data parameter)

> I've changed the code to utilize this parameter to pass the
> string. In addition, I've made the function calculate the string
> length on usage instead, to reduce the parameters needed to what
> the callback-interface supplies.

Usually, commit messages are held in a more imperative form than a 
subjective one:

	Utilize this parameter to pass the string.

> diff --git a/connect.c b/connect.c
> index 2f23ab3..98fbaea 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -371,14 +371,13 @@ static void git_tcp_connect(int fd[2], char *host, int flags)
>  	fd[1] = dup(sockfd);
>  }
>  
> -
>  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)
> +		void *data)
>  {
> +	const char *rhost_name = data;
> +	const size_t rhost_len = strlen(rhost_name);
> +

git_proxy_command_options is called for each and every config variable.  
The idea of having the length in a local variable was to avoid 
recalculating the length each and every time.  I think I'd actually use a 
strbuf for that.

BTW I would not rename "cb", as it is only distracting the reader of the 
patch.

But I like the idea of your patch (as you can see from me replying ;-)

Thanks,
Dscho

Re: Removed unnecessary use of global variables.

[Johannes Schindelin]

Hi,

On Tue, 10 Mar 2009, Erik Faye-Lund wrote:

> Sorry about the whole double-post thing, this whole submitting patches 
> over email thing is new to me, and I'm making too many mistakes right 
> now. Hopefully, I'll improve in the future ;)

No need to be sorry.  Thanks for your contribution!

Ciao,
Dscho

Re: [PATCH] Removed unnecessary use of global variables.

[Nanako Shiraishi]

Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:

> Hi,
>
> On Wed, 11 Mar 2009, Erik Faye-Lund wrote:
>
>> git_config() now takes a third data-parameter that is passed back
>> to the callback-function. At the time this code was written, that
>> parameter did not exist, so a somewhat nasty (but by all means
>> correct) use of global variables was introduced. In commit
>> ef90d6d4208a5130185b04f06e5f90a5f9959fe3 Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> introduced a parameter for similar
>> purposes.
>
> We tend to quote commits in this form: ef90d6d(Provide git_config with a 
> callback-data parameter)

Your review comments are a subset of the ones Junio sent about 8 hours before you did, and are almost identical except for the comment on the subject line yours didn't have. I'm curious about two things.

1. Are you and Junio one same person, and if so what made you change your mind during these 8 hours ;-)?

2. Junio said "ef90d6d (Provide git_config with a callback-data parameter, 2008-05-14)" and yours is slightly different. Both are equally readable but I think it would help to make sure everybody uses the same format within one project. Do we need a helper command that everybody can use?

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH] Removed unnecessary use of global variables.

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> 1. Are you and Junio one same person, and if so what made you change your mind during these 8 hours ;-)?

Last year during Gittogether, Dscho and I were seen in a room sitting
parhaps 4 meters apart; I suspect we are different people.

http://picasaweb.google.com/dsymonds/GitTogether08#5263025230667494002

> 2. Junio said "ef90d6d (Provide git_config with a callback-data parameter, 2008-05-14)" and yours is slightly different. Both are equally readable but I think it would help to make sure everybody uses the same format within one project. Do we need a helper command that everybody can use?

I use these two aliases while I am writing e-mails quite often:

[alias]
	one = "!sh -c 'git show -s --pretty=\"format:%h (%s, %ai\" \"$@\" | sed -e \"s/ [012][0-9]:[0-5][0-9]:[0-5][0-9] [-+][0-9][0-9][0-9][0-9]$/)/\"' -"
	who = "!sh -c 'git log -1 --pretty=\"format:%an <%ae>\" --author=\"$1\"' -"