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

On Fri, Feb 28, 2014 at 9:17 AM, 孙赫 [off-list ref] wrote:

2014-02-28 21:12 GMT+08:00 Eric Sunshine [via git]
[off-list ref]:

quoted

On Fri, Feb 28, 2014 at 4:46 AM, Eric Sunshine <[hidden email]> wrote:

quoted

On Fri, Feb 28, 2014 at 3:28 AM, Sun He <[hidden email]> wrote:

quoted

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().

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.