2014-03-01 4:42 GMT+08:00 Eric Sunshine <sunshine@xxxxxxxxxxxxxx>:
> On Fri, Feb 28, 2014 at 9:17 AM, 孙赫 <sunheehnus@xxxxxxxxx> wrote:
>> 2014-02-28 21:12 GMT+08:00 Eric Sunshine [via git]
>> <ml-node+s661346n7604500h1@xxxxxxxxxxxxx>:
>>> 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.
>>>
>>
>> Thank you for your explaination. I get the point.
>> And I think if it is proved that the strbuf way is measurably slower.
>> We should add a check for the length of string before we sprintf().
>
> I'm not sure what you mean by checking the string length before sprintf().
>
That's because one is not certain of the length of get_object_directory()
Micheal Mhaggerty<mhagger@xxxxxxxxxxxx> explained this to me.
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().
It is the same as yours about the space and time costs. ^_^
> My point was that if there are multiple ways of solving the same
> problem, it can be helpful for reviewers if your commit message
> explains why the solution you chose is better than the others.
>
> Slowness and/or cleanliness of API were just examples you might use in
> your commit message for justifying why you chose one approach over
> another.
OK, OK, Got it. Thank you very much.
Cheers,
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