Re: [PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname
2014-02-28 21:12 GMT+08:00 Eric Sunshine [via git]
ml-node+s661346n760450...@n2.nabble.com:
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().
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
Re: [PATCH] OPTION_CMDMODE should be used when not accept an argument, and OPTION_NUMBER is of special type. So change the mode to OPTION_CMDMODE
I am not sure if this is a bug. I need your help to find out it. Cheers, He Sun 2014-02-28 22:29 GMT+08:00 Sun He sunheeh...@gmail.com: Signed-off-by: Sun He sunheeh...@gmail.com --- parse-options.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parse-options.c b/parse-options.c index 7b8d3fa..59a52b0 100644 --- a/parse-options.c +++ b/parse-options.c @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts) case OPTION_NEGBIT: case OPTION_SET_INT: case OPTION_SET_PTR: - case OPTION_NUMBER: + case OPTION_CMDMODE: if ((opts-flags PARSE_OPT_OPTARG) || !(opts-flags PARSE_OPT_NOARG)) err |= optbug(opts, should not accept an argument); -- 1.9.0.138.g2de3478.dirty --- I came across this protential bug. According to parse-options.h OPTION_CMDMODE is an option with noarguments and OPTION_NUMBER is special type option. Thanks, He Sun -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] OPTION_CMDMODE should be used when not accept an argument, and OPTION_NUMBER is of special type. So change the mode to OPTION_CMDMODE
2014-02-28 22:43 GMT+08:00 Duy Nguyen [via git] ml-node+s661346n7604517...@n2.nabble.com: Way too long subject line. Keep it within 70-75 chars. The rest could be put in the body. On Fri, Feb 28, 2014 at 9:32 PM, 孙赫 [hidden email] wrote: I am not sure if this is a bug. I need your help to find out it. Tip:git has a wonderful history (most of it anyway). Try git log --patch parse-options.[ch] to understand parse-options evolution. Add -SOPTION_NUMBER (or -SOPTION_CMDMODE) to limit to only commits whose diff contains that keyword. -- Duy -- Got it, Thanks 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-OPTION-CMDMODE-should-be-used-when-not-accept-an-argument-and-OPTION-NUMBER-is-of-special-typeE-tp7604513p7604517.html To start a new topic under git, email ml-node+s661346n661346...@n2.nabble.com To unsubscribe from git, click here. NAML -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Fwd: [PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname
-- Forwarded message --
From: 孙赫 sunheeh...@gmail.com
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+s661346n7604473...@n2.nabble.com
2014-02-28 17:47 GMT+08:00 Eric Sunshine [via git]
ml-node+s661346n7604473...@n2.nabble.com:
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
