[PATCH 1/2] code cleanup in sha1_file.c: xmalloc -> xcalloc

Previous thread: [StGit PATCH] Add mergetool support to the classic StGit infrastructure by Catalin Marinas on Tuesday, February 10, 2009 - 7:14 am. (3 messages)

Next thread: git-svn: broken symlink check fetches wrong revision from svn (possible regression) by Anton Gyllenberg on Tuesday, February 10, 2009 - 7:51 am. (1 message)
From: Guanqun Lu
Date: Tuesday, February 10, 2009 - 11:43 pm

'xmalloc' followed immediately by 'memset' is replaced
with 'xcalloc', and a simple grep in this project seems
to show that it's the only place.

Signed-off-by: Guanqun Lu <guanqun.lu@gmail.com>
---
 sha1_file.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 8868b80..93e5fc0 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -858,8 +858,7 @@ unsigned char* use_pack(struct packed_git *p,
 
 static struct packed_git *alloc_packed_git(int extra)
 {
-	struct packed_git *p = xmalloc(sizeof(*p) + extra);
-	memset(p, 0, sizeof(*p));
+	struct packed_git *p = xcalloc(1, sizeof(*p) + extra);
 	p->pack_fd = -1;
 	return p;
 }
-- 
1.6.1.2.392.gb04d1

--

From: Guanqun Lu
Date: Tuesday, February 10, 2009 - 11:43 pm

Post-conditions when these functions return successfully:
                    lk->fd == -1?    lk->filename[0] == '\0'?
close_lock_file()       yes                 no
commit_lock_file()      yes                 yes
rollback_lock_file()    no*                 yes

[*] This commit changes this 'no' in rollback_lock_file() to 'yes',
which achieves more robust and unified interface.

Signed-off-by: Guanqun Lu <guanqun.lu@gmail.com>
---
 lockfile.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 021c337..44e5253 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -243,8 +243,10 @@ int commit_locked_index(struct lock_file *lk)
 void rollback_lock_file(struct lock_file *lk)
 {
 	if (lk->filename[0]) {
-		if (lk->fd >= 0)
+		if (lk->fd >= 0) {
 			close(lk->fd);
+			lk->fd = -1;
+		}
 		unlink(lk->filename);
 	}
 	lk->filename[0] = 0;
-- 
1.6.1.2.392.gb04d1

--

From: Junio C Hamano
Date: Tuesday, February 10, 2009 - 8:29 am

Is there a broken caller this patch fixes?  IOW, is there a codepath that
can call rollback and then later notices lk->fd is not negative and uses
--

From: Brandon Casey
Date: Tuesday, February 10, 2009 - 9:29 am

I previously scanned through and did this.  I left this one as is
because the extra part is about as large as the sizeof(*p) part.  69 and
72 bytes respectively on 32-bit.  The extra part is always filled in
immediately by callers.  It's only called once for each pack so it's
not performance critical, so maybe your patch is worth it since it is
simpler?

-brandon
--

From: Junio C Hamano
Date: Tuesday, February 10, 2009 - 9:43 am

But isn't this memset() done only for the initial part of the allocated
area, not the whole thing?  You are not cleaning up but changing what it
--

Previous thread: [StGit PATCH] Add mergetool support to the classic StGit infrastructure by Catalin Marinas on Tuesday, February 10, 2009 - 7:14 am. (3 messages)

Next thread: git-svn: broken symlink check fetches wrong revision from svn (possible regression) by Anton Gyllenberg on Tuesday, February 10, 2009 - 7:51 am. (1 message)