---------- Forwarded message ----------
From: 孙赫 <sunheehnus@xxxxxxxxx>
Date: 2014-02-28 21:37 GMT+08:00
Subject: Re: [PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to
use a strbuf for handling packname
To: "Eric Sunshine [via git]" <ml-node+s661346n7604473h64@xxxxxxxxxxxxx>
2014-02-28 17:47 GMT+08:00 Eric Sunshine [via git]
<ml-node+s661346n7604473h64@xxxxxxxxxxxxx>:
> On Fri, Feb 28, 2014 at 3:28 AM, Sun He <[hidden email]> wrote:
>> Signed-off-by: Sun He <[hidden email]>
>> ---
>
> Nicely done.
>
> Due to the necessary changes to finish_tmp_packfile(), the focus of
> this patch has changed slightly from that suggested by the
> microproject. It's really now about finish_tmp_packfile() rather than
> finish_bulk_checkin(). As such, it may make sense to adjust the patch
> subject to reflect this. For instance:
>
> Subject: finish_tmp_packfile(): use strbuf for pathname construction
>
That's great, I will adjust it as you suggested.
> More comments below.
>
> ]> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
>
>> index c733379..72fb82b 100644
>> --- a/builtin/pack-objects.c
>> +++ b/builtin/pack-objects.c
>> @@ -803,7 +803,7 @@ static void write_pack_file(void)
>>
>> if (!pack_to_stdout) {
>> struct stat st;
>> - char tmpname[PATH_MAX];
>> + struct strbuf tmpname = STRBUF_INIT;
>>
>> /*
>> * Packs are runtime accessed in their mtime
>> @@ -823,26 +823,22 @@ static void write_pack_file(void)
>> utb.modtime = --last_mtime;
>> if (utime(pack_tmp_name, &utb) < 0)
>> warning("failed utime() on %s:
>> %s",
>> - tmpname, strerror(errno));
>> + pack_tmp_name,
>> strerror(errno));
>
> Good catch finding this bug (as your commentary below states).
> Ideally, each conceptual change should be presented distinctly from
> others, so this bug should have its own patch (even though it's just a
> one-liner).
>
OK. Should I send this patch?
>> }
>>
>> - /* Enough space for "-<sha-1>.pack"? */
>> - if (sizeof(tmpname) <= strlen(base_name) + 50)
>> - die("pack base name '%s' too long",
>> base_name);
>> - snprintf(tmpname, sizeof(tmpname), "%s-",
>> base_name);
>> + strbuf_addf(&tmpname, "%s-", base_name);
>>
>> if (write_bitmap_index) {
>> bitmap_writer_set_checksum(sha1);
>>
>> bitmap_writer_build_type_index(written_list, nr_written);
>> }
>>
>> - finish_tmp_packfile(tmpname, pack_tmp_name,
>> + finish_tmp_packfile(&tmpname, pack_tmp_name,
>> written_list, nr_written,
>> &pack_idx_opts, sha1);
>>
>> if (write_bitmap_index) {
>> - char *end_of_name_prefix =
>> strrchr(tmpname, 0);
>> - sprintf(end_of_name_prefix, "%s.bitmap",
>> sha1_to_hex(sha1));
>> + strbuf_addf(&tmpname, "%s.bitmap"
>> ,sha1_to_hex(sha1));
>>
>> stop_progress(&progress_state);
>>
>> diff --git a/pack-write.c b/pack-write.c
>> index 9b8308b..2204aa9 100644
>> --- a/pack-write.c
>> +++ b/pack-write.c
>> @@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char
>> **pack_tmp_name)
>> return sha1fd(fd, *pack_tmp_name);
>> }
>>
>> -void finish_tmp_packfile(char *name_buffer,
>> +void finish_tmp_packfile(struct strbuf *name_buffer,
>> const char *pack_tmp_name,
>> struct pack_idx_entry **written_list,
>> uint32_t nr_written,
>> @@ -344,7 +344,7 @@ void finish_tmp_packfile(char *name_buffer,
>> unsigned char sha1[])
>> {
>> const char *idx_tmp_name;
>> - char *end_of_name_prefix = strrchr(name_buffer, 0);
>> + int pre_len = name_buffer->len;
>
> Minor: Perhaps basename_len (or some such) would convey a bit more
> meaning than pre_len.
>
It's pretty good to work with you next suggestion. THX
>> if (adjust_shared_perm(pack_tmp_name))
>> die_errno("unable to make temporary pack file readable");
>> @@ -354,17 +354,21 @@ void finish_tmp_packfile(char *name_buffer,
>> if (adjust_shared_perm(idx_tmp_name))
>> die_errno("unable to make temporary index file readable");
>>
>> - sprintf(end_of_name_prefix, "%s.pack", sha1_to_hex(sha1));
>> - free_pack_by_name(name_buffer);
>> + strbuf_addf(name_buffer, "%s.pack", sha1_to_hex(sha1));
>> + free_pack_by_name(name_buffer->buf);
>>
>> - if (rename(pack_tmp_name, name_buffer))
>> + if (rename(pack_tmp_name, name_buffer->buf))
>> die_errno("unable to rename temporary pack file");
>>
>> - sprintf(end_of_name_prefix, "%s.idx", sha1_to_hex(sha1));
>> - if (rename(idx_tmp_name, name_buffer))
>> + /* remove added suffix string(sha1.pack) */
>> + strbuf_remove(name_buffer, pre_len, name_buffer->len - pre_len);
>
> Since you are merely truncating the buffer back to pre_len, perhaps
>
> strbuf_setlen(name_buffer, pre_len)
>
> would be more idiomatic and easier to read (and would allow you to
> drop the comment).
>
Yeah, yeah. It's more elegent. I will fix it later.
>> +
>> + strbuf_addf(name_buffer, "%s.idx", sha1_to_hex(sha1));
>> + if (rename(idx_tmp_name, name_buffer->buf))
>> die_errno("unable to rename temporary index file");
>>
>> - *end_of_name_prefix = '\0';
>> + /* remove added suffix string(sha1.idx) */
>> + strbuf_remove(name_buffer, pre_len, name_buffer->len - pre_len);
>
> Ditto.
>
>> free((void *)idx_tmp_name);
>> }
>> --
>> 1.9.0.138.g2de3478.dirty
>>
>> Hello,
>> This is my patch for the GSoC microproject #2
>>
>> I follow the suggestion of Junio to use the strbuf_addf to deal with this
>> thing.
>> And the usage of strbuf_addf needs to change the function
>> finish_tmp_packfile, I search all over the source code ($ find .| xargs grep
>> "finish_tmp_packfile")
>> The following files contains finish_tmp_packfile:
>> bulk-checkin.c
>> builtin/pack-object.c
>> pack-write.c
>> pack.h
>> And I do some change to fix them.
>> I changed my vimrc so that tabs will not be changed into spaces
>> automatically.
>>
>> And I came across a bug when I was doing my microproject.
>> position: builtin/pack-objects.c: write_pack_file: if(!pack_to_stdout):
>> first else in it
>> warning() function used an uninitialized array, and I changed it to
>> pack_tmp_name.
>>
>> Thank you all for all your suggestions.
>
> This section explaining your changes is important for the ongoing
> email discussion, however, it is customary (and "git am"-friendly) to
> place these notes just below the "---" line following your signoff
> near the top of the email.
> --
Got it.
Thank you very very much for your code review and great suggestions.
He Sun
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html