[PATCH] Allow git_author|committer_info be called in the same expression

[PATCH] Allow git_author|committer_info be called in the same expression

[Alex Riesen]

[-- Attachment #1: Type: text/plain, Size: 1180 bytes --]

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 ident.c |   49 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 32 insertions(+), 17 deletions(-)

On 7/31/07, Kristian Høgsberg <krh@redhat.com> wrote:
> There's number of buffers that don't get freed: the strbuf, the commit
> message buffer, and the strdup'ed author and committer info.  All the
> leaks are not critical since the process exits immediately.  As for the
> strbuf leak, I was thinking about renaming strbuf_begin to strbuf_reset
> and making it public[1], which will then be used for freeing up strbuf
> memory.  The message buffer leak should be fixed by adding a
> strbuf_read_fd() that just reads it straight into the strbuf.  The
> xstrdup's are necessary because fmt_ident uses a static buffer (thanks,
> test case :).  We could add rotating static buffers for fmt_ident like
> git_path and avoid the strdups, but again, the leaks are not critical.

Ach, that's why... Junio, how about this then? I'd even suggest adding
the buffer argument to fmt_ident and use it everywhere (ATM there is
only one user outside of ident.c: builtin-blame.c).
This one is applicable immediately, though

[-- Attachment #2: 0001-Allow-git_author-committer_info-be-called-in-the-same.txt --]
[-- Type: text/plain, Size: 2862 bytes --]

From 0c86739aa9f5db19ebcd2bc041622c317611c87f Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Tue, 31 Jul 2007 18:48:23 +0200
Subject: [PATCH] Allow git_author|committer_info be called in the same expression


Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 ident.c |   49 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/ident.c b/ident.c
index 6612d17..0d59090 100644
--- a/ident.c
+++ b/ident.c
@@ -192,10 +192,10 @@ static const char *env_hint =
 "Add --global to set your account\'s default\n"
 "\n";
 
-const char *fmt_ident(const char *name, const char *email,
-		      const char *date_str, int error_on_no_name)
+#define FMT_IDENT_BUFSIZE 1000
+static char *fmt_ident_buf(char *buffer, const char *name, const char *email,
+			   const char *date_str, int error_on_no_name)
 {
-	static char buffer[1000];
 	char date[50];
 	int i;
 
@@ -227,29 +227,44 @@ const char *fmt_ident(const char *name, const char *email,
 	if (date_str)
 		parse_date(date_str, date, sizeof(date));
 
-	i = copy(buffer, sizeof(buffer), 0, name);
-	i = add_raw(buffer, sizeof(buffer), i, " <");
-	i = copy(buffer, sizeof(buffer), i, email);
-	i = add_raw(buffer, sizeof(buffer), i, "> ");
-	i = copy(buffer, sizeof(buffer), i, date);
-	if (i >= sizeof(buffer))
+	i = copy(buffer, FMT_IDENT_BUFSIZE, 0, name);
+	i = add_raw(buffer, FMT_IDENT_BUFSIZE, i, " <");
+	i = copy(buffer, FMT_IDENT_BUFSIZE, i, email);
+	i = add_raw(buffer, FMT_IDENT_BUFSIZE, i, "> ");
+	i = copy(buffer, FMT_IDENT_BUFSIZE, i, date);
+	if (i >= FMT_IDENT_BUFSIZE)
 		die("Impossibly long personal identifier");
 	buffer[i] = 0;
 	return buffer;
 }
 
+const char *fmt_ident(const char *name, const char *email,
+		      const char *date_str, int error_on_no_name)
+{
+	static char buffer[FMT_IDENT_BUFSIZE];
+	return fmt_ident_buf(buffer, name, email, date_str, error_on_no_name);
+}
+
 const char *git_author_info(int error_on_no_name)
 {
-	return fmt_ident(getenv("GIT_AUTHOR_NAME"),
-			 getenv("GIT_AUTHOR_EMAIL"),
-			 getenv("GIT_AUTHOR_DATE"),
-			 error_on_no_name);
+	static char *buffer;
+	if (!buffer)
+		buffer = xmalloc(FMT_IDENT_BUFSIZE);
+	return fmt_ident_buf(buffer,
+			     getenv("GIT_AUTHOR_NAME"),
+			     getenv("GIT_AUTHOR_EMAIL"),
+			     getenv("GIT_AUTHOR_DATE"),
+			     error_on_no_name);
 }
 
 const char *git_committer_info(int error_on_no_name)
 {
-	return fmt_ident(getenv("GIT_COMMITTER_NAME"),
-			 getenv("GIT_COMMITTER_EMAIL"),
-			 getenv("GIT_COMMITTER_DATE"),
-			 error_on_no_name);
+	static char *buffer;
+	if (!buffer)
+		buffer = xmalloc(FMT_IDENT_BUFSIZE);
+	return fmt_ident_buf(buffer,
+			     getenv("GIT_COMMITTER_NAME"),
+			     getenv("GIT_COMMITTER_EMAIL"),
+			     getenv("GIT_COMMITTER_DATE"),
+			     error_on_no_name);
 }
-- 
1.5.3.rc3.132.g39179

Re: [PATCH] Allow git_author|committer_info be called in the same expression

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

>  const char *git_committer_info(int error_on_no_name)
>  {
> -	return fmt_ident(getenv("GIT_COMMITTER_NAME"),
> -			 getenv("GIT_COMMITTER_EMAIL"),
> -			 getenv("GIT_COMMITTER_DATE"),
> -			 error_on_no_name);
> +	static char *buffer;
> +	if (!buffer)
> +		buffer = xmalloc(FMT_IDENT_BUFSIZE);

It is not like we are aiming to run on embedded devices, I think
trying to save static memory when the function is not called
by doing things like this is not worth it.  Why not just:

	static char buffer[FMT_IDENT_BUFSIZE];

instead?

In any case, I'd really like to slow things down and ask
everybody to concentrate on finding and fixing bugs in what are
scheduled for 1.5.3 for a week or so.

That means the current "master".

I did not merge Johannes's work tree rationalization patch to
"master" yet (it is now part of "next"), but I would like to
have it in, as it makes the code much easier to read compared to
the work-tree changes currently in "master".  Even though the
one currently in "master" does not seem to regress the normal
usage of not using GIT_WORK_TREE environment, we would be better
off if a new feature is released in a shape that is maintainable.

Other than that topic, everything on "next" will be post 1.5.3.

Please.

I'd like to do -rc4 this weekend, and then 1.5.3 final by mid
August at the latest, but hopefully sooner.

As to parts of the system that other people are primarily in
charge of, git-gui 0.8.0 is already in as promised, updates to
gitk were merged, and we've had a nice set of improvements from
Jakub on gitweb already.  Bruce seems to have a beginning of the
user manual reorganization, but I am getting the feeling it is
not ready yet.

Re: [PATCH] Allow git_author|committer_info be called in the same expression

[Alex Riesen]

On 8/1/07, Junio C Hamano <gitster@pobox.com> wrote:
> "Alex Riesen" <raa.lkml@gmail.com> writes:
>
> >  const char *git_committer_info(int error_on_no_name)
> >  {
> > -     return fmt_ident(getenv("GIT_COMMITTER_NAME"),
> > -                      getenv("GIT_COMMITTER_EMAIL"),
> > -                      getenv("GIT_COMMITTER_DATE"),
> > -                      error_on_no_name);
> > +     static char *buffer;
> > +     if (!buffer)
> > +             buffer = xmalloc(FMT_IDENT_BUFSIZE);
>
> It is not like we are aiming to run on embedded devices, I think

Actually, I am planning to run it on Neo1973 (Openmoko),
which is quite embedded with its 128mb.

> trying to save static memory when the function is not called
> by doing things like this is not worth it.  Why not just:
>
>         static char buffer[FMT_IDENT_BUFSIZE];
>
> instead?

Just a custom. This is a big bugger, err... buffer.

> As to parts of the system that other people are primarily in
> charge of, git-gui 0.8.0 is already in as promised, updates to
> gitk were merged, and we've had a nice set of improvements from

BTW, gitk. Have you seen the gitk patches regarding errors in non-GIT
and not-yet-GIT directory?