Re: [PATCH] rewrite finish_bulk_checkin() using strbuf
2014-03-01 15:18 GMT+08:00 Faiz Kothari faiz.of...@gmail.com: Hi, Yup, at that position. I don't know, but it failed a few tests on my machine related to bitmap. Another thing to use would be strbuf_splice() Eric Sunshinesunsh...@sunshineco.com has came up with a more elegant way to finish this task. That's using strbuf_setlen() instead of strbuf_remove(). Because of the unstable network of this afternoon. The former email is sent without the above information. Sorry about it. I find out that you didn't attach strbuf_remove() in your finish_bulk_checkin(). That may cause problems not only related to bitmap, because the input packname is different from the output packname.. Cheers, He Sun Anyways, no worries :) Cheers, Faiz On Sat, Mar 1, 2014 at 12:40 PM, He Sun sunheeh...@gmail.com wrote: 2014-03-01 14:46 GMT+08:00 Faiz Kothari faiz.of...@gmail.com: From: Faiz Kotahri faiz.of...@gmail.com Signed-off-by: Faiz Kothari faiz.of...@gmail.com --- Sticking with implementation involving changing the prototype for pack-write.c:finish_tmp_packfile() Fixing a small bug in Sun He's implementation which caused a fail in some tests. builtin/pack-objects.c | 25 - bulk-checkin.c |9 ++--- pack-write.c | 19 ++- pack.h |3 ++- 4 files changed, 30 insertions(+), 26 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index c733379..4b59bba 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -20,6 +20,7 @@ #include streaming.h #include thread-utils.h #include pack-bitmap.h +#include strbuf.h static const char *pack_usage[] = { N_(git pack-objects --stdout [options...] [ ref-list | object-list]), @@ -803,8 +804,8 @@ static void write_pack_file(void) if (!pack_to_stdout) { struct stat st; - char tmpname[PATH_MAX]; - + struct strbuf tmpname = STRBUF_INIT; + int ini_length; /* * Packs are runtime accessed in their mtime * order since newer packs are more likely to contain @@ -823,26 +824,24 @@ 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)); } - /* 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); + ini_length = tmpname.len; 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_remove(tmpname, ini_length, tmpname.len - ini_length); Is this the position where you think may import bugs? I think it should be the duty of finish_tmp_packfile to ensure that the tmpname is the same as it is input as a param. And in my original code, I have used strbuf_remove() at the end of finish_tmp_packfile. There is a more elegant way to finish this task.[According to ] + strbuf_addf(tmpname, %s.bitmap, sha1_to_hex(sha1)); stop_progress(progress_state); @@ -851,10 +850,10 @@ static void write_pack_file(void) bitmap_writer_select_commits(indexed_commits, indexed_commits_nr, -1); bitmap_writer_build(to_pack); bitmap_writer_finish(written_list, nr_written, -tmpname, write_bitmap_options); +tmpname.buf, write_bitmap_options); write_bitmap_index = 0;
Re: [PATCH] rewrite finish_bulk_checkin() using strbuf
On Sat, Mar 1, 2014 at 4:33 PM, He Sun sunheeh...@gmail.com wrote: 2014-03-01 15:18 GMT+08:00 Faiz Kothari faiz.of...@gmail.com: Hi, Yup, at that position. I don't know, but it failed a few tests on my machine related to bitmap. Another thing to use would be strbuf_splice() Eric Sunshinesunsh...@sunshineco.com has came up with a more elegant way to finish this task. That's using strbuf_setlen() instead of strbuf_remove(). Because of the unstable network of this afternoon. The former email is sent without the above information. Sorry about it. You mean first use strbuf_setlen() to reduce len and then add .whatever to it? strbuf_splice can do strbuf_remove() and add string in one shot as per the doc. If using strbuf_setlen() is still better, I'll implement using that :) I find out that you didn't attach strbuf_remove() in your finish_bulk_checkin(). That may cause problems not only related to bitmap, because the input packname is different from the output packname.. I am not sure how that is causing the problem, because tests run just fine. I'll still look into it. Thanks for pointing that out :) Cheers, Faiz -- 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
[PATCH] rewrite finish_bulk_checkin() using strbuf
From: Faiz Kotahri faiz.of...@gmail.com Signed-off-by: Faiz Kothari faiz.of...@gmail.com --- Sticking with implementation involving changing the prototype for pack-write.c:finish_tmp_packfile() Fixing a small bug in Sun He's implementation which caused a fail in some tests. builtin/pack-objects.c | 25 - bulk-checkin.c |9 ++--- pack-write.c | 19 ++- pack.h |3 ++- 4 files changed, 30 insertions(+), 26 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index c733379..4b59bba 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -20,6 +20,7 @@ #include streaming.h #include thread-utils.h #include pack-bitmap.h +#include strbuf.h static const char *pack_usage[] = { N_(git pack-objects --stdout [options...] [ ref-list | object-list]), @@ -803,8 +804,8 @@ static void write_pack_file(void) if (!pack_to_stdout) { struct stat st; - char tmpname[PATH_MAX]; - + struct strbuf tmpname = STRBUF_INIT; + int ini_length; /* * Packs are runtime accessed in their mtime * order since newer packs are more likely to contain @@ -823,26 +824,24 @@ 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)); } - /* 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); + ini_length = tmpname.len; 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_remove(tmpname, ini_length, tmpname.len - ini_length); + strbuf_addf(tmpname, %s.bitmap, sha1_to_hex(sha1)); stop_progress(progress_state); @@ -851,10 +850,10 @@ static void write_pack_file(void) bitmap_writer_select_commits(indexed_commits, indexed_commits_nr, -1); bitmap_writer_build(to_pack); bitmap_writer_finish(written_list, nr_written, -tmpname, write_bitmap_options); +tmpname.buf, write_bitmap_options); write_bitmap_index = 0; } - + strbuf_release(tmpname); free(pack_tmp_name); puts(sha1_to_hex(sha1)); } diff --git a/bulk-checkin.c b/bulk-checkin.c index 118c625..248454c 100644 --- a/bulk-checkin.c +++ b/bulk-checkin.c @@ -4,6 +4,7 @@ #include bulk-checkin.h #include csum-file.h #include pack.h +#include strbuf.h static int pack_compression_level = Z_DEFAULT_COMPRESSION; @@ -23,7 +24,7 @@ static struct bulk_checkin_state { static void finish_bulk_checkin(struct bulk_checkin_state *state) { unsigned char sha1[20]; - char packname[PATH_MAX]; + struct strbuf packname = STRBUF_INIT; int i; if (!state-f) @@ -43,16 +44,18 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state) close(fd); } - sprintf(packname, %s/pack/pack-, get_object_directory()); - finish_tmp_packfile(packname, state-pack_tmp_name, + strbuf_addf(packname, %s/pack/pack-, get_object_directory()); + finish_tmp_packfile(packname, state-pack_tmp_name, state-written, state-nr_written, state-pack_idx_opts, sha1); + for (i = 0; i
Re: [PATCH] rewrite finish_bulk_checkin() using strbuf
2014-03-01 14:46 GMT+08:00 Faiz Kothari faiz.of...@gmail.com: From: Faiz Kotahri faiz.of...@gmail.com Signed-off-by: Faiz Kothari faiz.of...@gmail.com --- Sticking with implementation involving changing the prototype for pack-write.c:finish_tmp_packfile() Fixing a small bug in Sun He's implementation which caused a fail in some tests. builtin/pack-objects.c | 25 - bulk-checkin.c |9 ++--- pack-write.c | 19 ++- pack.h |3 ++- 4 files changed, 30 insertions(+), 26 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index c733379..4b59bba 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -20,6 +20,7 @@ #include streaming.h #include thread-utils.h #include pack-bitmap.h +#include strbuf.h static const char *pack_usage[] = { N_(git pack-objects --stdout [options...] [ ref-list | object-list]), @@ -803,8 +804,8 @@ static void write_pack_file(void) if (!pack_to_stdout) { struct stat st; - char tmpname[PATH_MAX]; - + struct strbuf tmpname = STRBUF_INIT; + int ini_length; /* * Packs are runtime accessed in their mtime * order since newer packs are more likely to contain @@ -823,26 +824,24 @@ 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)); } - /* 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); + ini_length = tmpname.len; 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_remove(tmpname, ini_length, tmpname.len - ini_length); Is this the position where you think may import bugs? I think it should be the duty of finish_tmp_packfile to ensure that the tmpname is the same as it is input as a param. And in my original code, I have used strbuf_remove() at the end of finish_tmp_packfile. Eric Sunshine sunsh...@sunshineco.com came up with a more elegant way to finish this task. That's using strbuf_setlen() instead of strbuf_remove(). + strbuf_addf(tmpname, %s.bitmap, sha1_to_hex(sha1)); stop_progress(progress_state); @@ -851,10 +850,10 @@ static void write_pack_file(void) bitmap_writer_select_commits(indexed_commits, indexed_commits_nr, -1); bitmap_writer_build(to_pack); bitmap_writer_finish(written_list, nr_written, -tmpname, write_bitmap_options); +tmpname.buf, write_bitmap_options); write_bitmap_index = 0; } - + strbuf_release(tmpname); free(pack_tmp_name); puts(sha1_to_hex(sha1)); } diff --git a/bulk-checkin.c b/bulk-checkin.c index 118c625..248454c 100644 --- a/bulk-checkin.c +++ b/bulk-checkin.c @@ -4,6 +4,7 @@ #include bulk-checkin.h #include csum-file.h #include pack.h +#include strbuf.h static int pack_compression_level = Z_DEFAULT_COMPRESSION; @@ -23,7 +24,7 @@ static struct bulk_checkin_state { static void finish_bulk_checkin(struct bulk_checkin_state *state) { unsigned char sha1[20]; - char packname[PATH_MAX]; + struct strbuf packname = STRBUF_INIT; int i;
Re: [PATCH] rewrite finish_bulk_checkin() using strbuf
2014-03-01 14:46 GMT+08:00 Faiz Kothari faiz.of...@gmail.com: From: Faiz Kotahri faiz.of...@gmail.com Signed-off-by: Faiz Kothari faiz.of...@gmail.com --- Sticking with implementation involving changing the prototype for pack-write.c:finish_tmp_packfile() Fixing a small bug in Sun He's implementation which caused a fail in some tests. builtin/pack-objects.c | 25 - bulk-checkin.c |9 ++--- pack-write.c | 19 ++- pack.h |3 ++- 4 files changed, 30 insertions(+), 26 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index c733379..4b59bba 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -20,6 +20,7 @@ #include streaming.h #include thread-utils.h #include pack-bitmap.h +#include strbuf.h static const char *pack_usage[] = { N_(git pack-objects --stdout [options...] [ ref-list | object-list]), @@ -803,8 +804,8 @@ static void write_pack_file(void) if (!pack_to_stdout) { struct stat st; - char tmpname[PATH_MAX]; - + struct strbuf tmpname = STRBUF_INIT; + int ini_length; /* * Packs are runtime accessed in their mtime * order since newer packs are more likely to contain @@ -823,26 +824,24 @@ 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)); } - /* 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); + ini_length = tmpname.len; 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_remove(tmpname, ini_length, tmpname.len - ini_length); Is this the position where you think may import bugs? I think it should be the duty of finish_tmp_packfile to ensure that the tmpname is the same as it is input as a param. And in my original code, I have used strbuf_remove() at the end of finish_tmp_packfile. There is a more elegant way to finish this task.[According to ] + strbuf_addf(tmpname, %s.bitmap, sha1_to_hex(sha1)); stop_progress(progress_state); @@ -851,10 +850,10 @@ static void write_pack_file(void) bitmap_writer_select_commits(indexed_commits, indexed_commits_nr, -1); bitmap_writer_build(to_pack); bitmap_writer_finish(written_list, nr_written, -tmpname, write_bitmap_options); +tmpname.buf, write_bitmap_options); write_bitmap_index = 0; } - + strbuf_release(tmpname); free(pack_tmp_name); puts(sha1_to_hex(sha1)); } diff --git a/bulk-checkin.c b/bulk-checkin.c index 118c625..248454c 100644 --- a/bulk-checkin.c +++ b/bulk-checkin.c @@ -4,6 +4,7 @@ #include bulk-checkin.h #include csum-file.h #include pack.h +#include strbuf.h static int pack_compression_level = Z_DEFAULT_COMPRESSION; @@ -23,7 +24,7 @@ static struct bulk_checkin_state { static void finish_bulk_checkin(struct bulk_checkin_state *state) { unsigned char sha1[20]; - char packname[PATH_MAX]; + struct strbuf packname = STRBUF_INIT; int i; if (!state-f) @@ -43,16 +44,18 @@ static void finish_bulk_checkin(struct