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
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
