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

> On Fri, Feb 28, 2014 at 4:46 AM, Eric Sunshine <[hidden email]> wrote:
>
>> 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
>
> You may also want your commit message to explain why you chose this
> approach over other legitimate approaches. For instance, your change
> places extra burden on callers of finish_tmp_packfile() by leaking a
> detail of its implementation: namely that it wants to manipulate
> pathnames easily (via strbuf). An equally valid and more encapsulated
> approach would be for finish_tmp_packfile() to accept a 'const char *'
> and construct its own strbuf internally.
>
> If the extra strbuf creation in the alternate approach is measurably
> slower, then you could use that fact to justify your implementation
> choice in the commit message. On the other hand, if it's not
> measurably slower, then perhaps the more encapsulated approach with
> cleaner API might be preferable.
>

>> 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).
>>
>>>                         }
>>>
>>> -                       /* 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.
>>
>>>         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).
>>
>>> +
>>> +       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.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to [hidden email]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
> ________________________________
> If you reply to this email, your message will be added to the discussion
> below:
> http://git.661346.n2.nabble.com/PATCH-Rewrite-bulk-checkin-c-finish-bulk-checkin-to-use-a-strbuf-for-handling-packname-tp7604449p7604500.html
> To start a new topic under git, email
> ml-node+s661346n661346h27@n2.nabble.com
> To unsubscribe from git, click here.
> NAML