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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



---------- 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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]