[PATCH] GSoC2014 microprojects Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname

[PATCH] GSoC2014 microprojects Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname

[Sun He]

Signed-off-by: Sun He <sunheehnus@gmail.com>
---
 bulk-checkin.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 118c625..e3c7fb2 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -23,7 +23,8 @@ static struct bulk_checkin_state {
 static void finish_bulk_checkin(struct bulk_checkin_state *state)
 {
 	unsigned char sha1[20];
-	char packname[PATH_MAX];
+    char *packname;
+    struct strbuf sb;
 	int i;
 
 	if (!state->f)
@@ -43,6 +44,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)
 		close(fd);
 	}
 
+     /* 64-1 is more than the sum of len(sha1_to_hex(sha1)) and len(".pack") */
+    strbuf_init(&sb,strlen(get_object_directory())+64);
+    packname = sb.buf;
+
 	sprintf(packname, "%s/pack/pack-", get_object_directory());
 	finish_tmp_packfile(packname, state->pack_tmp_name,
 			    state->written, state->nr_written,
@@ -54,6 +59,9 @@ clear_exit:
 	free(state->written);
 	memset(state, 0, sizeof(*state));
 
+    /* release sb space */
+    strbuf_release(&sb);
+
 	/* Make objects we just wrote available to ourselves */
 	reprepare_packed_git();
 }
-- 
1.7.1

> Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname, and explain why this is useful.

char packname[PATH_MAX] (PATH_MAX=4096) in a local function costs too much space, it may cause stack overflow.
Using strbuf to deal with packname is more space-friendly.
I only use the API strbuf_init to alloc enough space for packname. I didn't use other APIs because I want to make the change as little as possible that I don't have to fix other functions which may import new bugs.
Because the space spared for sb is on heap, we must release it after it is not useful.

> Also check if the first argument of pack-write.c:finish_tmp_packfile() can be made const.

Tht first argument of pack-write.c:finish_tmp_packfile() can be made const because we didn't use *name_buffer to change anything.

Cheers,
He Sun

PS: 
Why I cannot sent email to git@vger.kernel.org via my Firefox?
Can you receive my former emails?
Thanks.

Re: [PATCH] GSoC2014 microprojects Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname

[Michael Haggerty]

On 02/27/2014 03:20 PM, Sun He wrote:
> Signed-off-by: Sun He <sunheehnus@gmail.com>
> ---
>  bulk-checkin.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 118c625..e3c7fb2 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -23,7 +23,8 @@ static struct bulk_checkin_state {
>  static void finish_bulk_checkin(struct bulk_checkin_state *state)
>  {
>  	unsigned char sha1[20];
> -	char packname[PATH_MAX];
> +    char *packname;
> +    struct strbuf sb;
>  	int i;
>  
>  	if (!state->f)
> @@ -43,6 +44,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)
>  		close(fd);
>  	}
>  
> +     /* 64-1 is more than the sum of len(sha1_to_hex(sha1)) and len(".pack") */
> +    strbuf_init(&sb,strlen(get_object_directory())+64);
> +    packname = sb.buf;

Why is packname still needed?  Can't you just use sb.buf wherever
packname was used before?  And then you can also rename "sb", which is a
mostly meaningless name, to "packname".

> +
>  	sprintf(packname, "%s/pack/pack-", get_object_directory());



>  	finish_tmp_packfile(packname, state->pack_tmp_name,
>  			    state->written, state->nr_written,
> @@ -54,6 +59,9 @@ clear_exit:
>  	free(state->written);
>  	memset(state, 0, sizeof(*state));
>  
> +    /* release sb space */
> +    strbuf_release(&sb);
> +
>  	/* Make objects we just wrote available to ourselves */
>  	reprepare_packed_git();
>  }
> 

Please also adhere to the CodingGuidelines.  Either you or your mailer
have used spaces rather than tabs for indentation.  We also usually use
spaces around "+".

The following comments belong earlier, just after the "---" line.

> -- 1.7.1
>> Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname, and explain why this is useful.
> 
> char packname[PATH_MAX] (PATH_MAX=4096) in a local function costs too much space, it may cause stack overflow.
> Using strbuf to deal with packname is more space-friendly.
> I only use the API strbuf_init to alloc enough space for packname. I didn't use other APIs because I want to make the change as little as possible that I don't have to fix other functions which may import new bugs.
> Because the space spared for sb is on heap, we must release it after it is not useful.

Saving stack space is nice, though given that it takes more time to
allocate space on the heap, it is nonetheless usually preferred to use
the stack for temporary variables of this size.

The problem has more to do with the fact that the old version fixes a
maximum length on the buffer, which could be a problem if one is not
certain of the length of get_object_directory().

The other point of strbuf is that you don't have to do the error-prone
bookkeeping yourself.  So it would be preferable to use strbuf_addf().

>> Also check if the first argument of pack-write.c:finish_tmp_packfile() can be made const.
> 
> Tht first argument of pack-write.c:finish_tmp_packfile() can be made const because we didn't use *name_buffer to change anything.

So, why don't you include a second patch making this change?

> Cheers,
> He Sun
> 
> PS: 
> Why I cannot sent email to git@vger.kernel.org via my Firefox?
> Can you receive my former emails?
> Thanks.

At least one earlier mail arrived at the list.  You can check the
mailing list archives at gmane.org to verify things like this.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/