* [PATCH] rewrite finish_bulk_checkin() using strbuf
@ 2014-03-01 6:46 Faiz Kothari
2014-03-01 7:10 ` He Sun
2014-03-01 7:10 ` He Sun
0 siblings, 2 replies; 5+ messages in thread
From: Faiz Kothari @ 2014-03-01 6:46 UTC (permalink / raw)
To: git, sunshine, sunheehnus; +Cc: Faiz Kotahri
From: Faiz Kotahri <faiz.off93@gmail.com>
Signed-off-by: Faiz Kothari <faiz.off93@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 < state->nr_written; i++)
free(state->written[i]);
clear_exit:
free(state->written);
memset(state, 0, sizeof(*state));
+ strbuf_release(&packname);
/* Make objects we just wrote available to ourselves */
reprepare_packed_git();
diff --git a/pack-write.c b/pack-write.c
index 9b8308b..60f5734 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -1,6 +1,7 @@
#include "cache.h"
#include "pack.h"
#include "csum-file.h"
+#include "strbuf.h"
void reset_pack_idx_option(struct pack_idx_option *opts)
{
@@ -336,7 +337,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 +345,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 ini_length = name_buffer->len;
if (adjust_shared_perm(pack_tmp_name))
die_errno("unable to make temporary pack file readable");
@@ -354,17 +355,17 @@ 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(end_of_name_prefix, "%s.idx", sha1_to_hex(sha1));
- if (rename(idx_tmp_name, name_buffer))
+ strbuf_remove(name_buffer, ini_length, name_buffer->len - ini_length);
+ strbuf_addf(name_buffer, "%s.idx", sha1_to_hex(sha1));
+
+ if (rename(idx_tmp_name, name_buffer->buf))
die_errno("unable to rename temporary index file");
- *end_of_name_prefix = '\0';
-
free((void *)idx_tmp_name);
}
diff --git a/pack.h b/pack.h
index 12d9516..0afe8d1 100644
--- a/pack.h
+++ b/pack.h
@@ -3,6 +3,7 @@
#include "object.h"
#include "csum-file.h"
+#include "strbuf.h"
/*
* Packed object header
@@ -91,6 +92,6 @@ extern int encode_in_pack_object_header(enum object_type, uintmax_t, unsigned ch
extern int read_pack_header(int fd, struct pack_header *);
extern struct sha1file *create_tmp_packfile(char **pack_tmp_name);
-extern void finish_tmp_packfile(char *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, struct pack_idx_option *pack_idx_opts, unsigned char sha1[]);
+extern void finish_tmp_packfile(struct strbuf *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, struct pack_idx_option *pack_idx_opts, unsigned char sha1[]);
#endif
--
1.7.9.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] rewrite finish_bulk_checkin() using strbuf
2014-03-01 6:46 [PATCH] rewrite finish_bulk_checkin() using strbuf Faiz Kothari
@ 2014-03-01 7:10 ` He Sun
2014-03-01 7:10 ` He Sun
1 sibling, 0 replies; 5+ messages in thread
From: He Sun @ 2014-03-01 7:10 UTC (permalink / raw)
To: Faiz Kothari, git
2014-03-01 14:46 GMT+08:00 Faiz Kothari <faiz.off93@gmail.com>:
> From: Faiz Kotahri <faiz.off93@gmail.com>
>
> Signed-off-by: Faiz Kothari <faiz.off93@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 <sunshine@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;
>
> 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 < state->nr_written; i++)
> free(state->written[i]);
>
> clear_exit:
> free(state->written);
> memset(state, 0, sizeof(*state));
> + strbuf_release(&packname);
>
> /* Make objects we just wrote available to ourselves */
> reprepare_packed_git();
> diff --git a/pack-write.c b/pack-write.c
> index 9b8308b..60f5734 100644
> --- a/pack-write.c
> +++ b/pack-write.c
> @@ -1,6 +1,7 @@
> #include "cache.h"
> #include "pack.h"
> #include "csum-file.h"
> +#include "strbuf.h"
>
> void reset_pack_idx_option(struct pack_idx_option *opts)
> {
> @@ -336,7 +337,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 +345,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 ini_length = name_buffer->len;
>
> if (adjust_shared_perm(pack_tmp_name))
> die_errno("unable to make temporary pack file readable");
> @@ -354,17 +355,17 @@ 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(end_of_name_prefix, "%s.idx", sha1_to_hex(sha1));
> - if (rename(idx_tmp_name, name_buffer))
> + strbuf_remove(name_buffer, ini_length, name_buffer->len - ini_length);
> + strbuf_addf(name_buffer, "%s.idx", sha1_to_hex(sha1));
> +
> + if (rename(idx_tmp_name, name_buffer->buf))
> die_errno("unable to rename temporary index file");
>
> - *end_of_name_prefix = '\0';
> -
> free((void *)idx_tmp_name);
> }
> diff --git a/pack.h b/pack.h
> index 12d9516..0afe8d1 100644
> --- a/pack.h
> +++ b/pack.h
> @@ -3,6 +3,7 @@
>
> #include "object.h"
> #include "csum-file.h"
> +#include "strbuf.h"
>
> /*
> * Packed object header
> @@ -91,6 +92,6 @@ extern int encode_in_pack_object_header(enum object_type, uintmax_t, unsigned ch
> extern int read_pack_header(int fd, struct pack_header *);
>
> extern struct sha1file *create_tmp_packfile(char **pack_tmp_name);
> -extern void finish_tmp_packfile(char *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, struct pack_idx_option *pack_idx_opts, unsigned char sha1[]);
> +extern void finish_tmp_packfile(struct strbuf *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, struct pack_idx_option *pack_idx_opts, unsigned char sha1[]);
>
> #endif
> --
> 1.7.9.5
>
Cheers,
He Sun
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rewrite finish_bulk_checkin() using strbuf
2014-03-01 6:46 [PATCH] rewrite finish_bulk_checkin() using strbuf Faiz Kothari
2014-03-01 7:10 ` He Sun
@ 2014-03-01 7:10 ` He Sun
[not found] ` <CAFbjVcmghtj80NetKoBeCKfMcECotiC=Q7ZmBR9ASuAtT_wiWg@mail.gmail.com>
1 sibling, 1 reply; 5+ messages in thread
From: He Sun @ 2014-03-01 7:10 UTC (permalink / raw)
To: Faiz Kothari, git
2014-03-01 14:46 GMT+08:00 Faiz Kothari <faiz.off93@gmail.com>:
> From: Faiz Kotahri <faiz.off93@gmail.com>
>
> Signed-off-by: Faiz Kothari <faiz.off93@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 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 < state->nr_written; i++)
> free(state->written[i]);
>
> clear_exit:
> free(state->written);
> memset(state, 0, sizeof(*state));
> + strbuf_release(&packname);
>
> /* Make objects we just wrote available to ourselves */
> reprepare_packed_git();
> diff --git a/pack-write.c b/pack-write.c
> index 9b8308b..60f5734 100644
> --- a/pack-write.c
> +++ b/pack-write.c
> @@ -1,6 +1,7 @@
> #include "cache.h"
> #include "pack.h"
> #include "csum-file.h"
> +#include "strbuf.h"
>
> void reset_pack_idx_option(struct pack_idx_option *opts)
> {
> @@ -336,7 +337,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 +345,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 ini_length = name_buffer->len;
>
> if (adjust_shared_perm(pack_tmp_name))
> die_errno("unable to make temporary pack file readable");
> @@ -354,17 +355,17 @@ 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(end_of_name_prefix, "%s.idx", sha1_to_hex(sha1));
> - if (rename(idx_tmp_name, name_buffer))
> + strbuf_remove(name_buffer, ini_length, name_buffer->len - ini_length);
> + strbuf_addf(name_buffer, "%s.idx", sha1_to_hex(sha1));
> +
> + if (rename(idx_tmp_name, name_buffer->buf))
> die_errno("unable to rename temporary index file");
>
> - *end_of_name_prefix = '\0';
> -
> free((void *)idx_tmp_name);
> }
> diff --git a/pack.h b/pack.h
> index 12d9516..0afe8d1 100644
> --- a/pack.h
> +++ b/pack.h
> @@ -3,6 +3,7 @@
>
> #include "object.h"
> #include "csum-file.h"
> +#include "strbuf.h"
>
> /*
> * Packed object header
> @@ -91,6 +92,6 @@ extern int encode_in_pack_object_header(enum object_type, uintmax_t, unsigned ch
> extern int read_pack_header(int fd, struct pack_header *);
>
> extern struct sha1file *create_tmp_packfile(char **pack_tmp_name);
> -extern void finish_tmp_packfile(char *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, struct pack_idx_option *pack_idx_opts, unsigned char sha1[]);
> +extern void finish_tmp_packfile(struct strbuf *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, struct pack_idx_option *pack_idx_opts, unsigned char sha1[]);
>
> #endif
> --
> 1.7.9.5
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rewrite finish_bulk_checkin() using strbuf
[not found] ` <CAFbjVcmghtj80NetKoBeCKfMcECotiC=Q7ZmBR9ASuAtT_wiWg@mail.gmail.com>
@ 2014-03-01 11:03 ` He Sun
2014-03-01 11:20 ` Faiz Kothari
0 siblings, 1 reply; 5+ messages in thread
From: He Sun @ 2014-03-01 11:03 UTC (permalink / raw)
To: Faiz Kothari, git, Eric Sunshine
2014-03-01 15:18 GMT+08:00 Faiz Kothari <faiz.off93@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 Sunshine<sunshine@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 <sunheehnus@gmail.com> wrote:
>> 2014-03-01 14:46 GMT+08:00 Faiz Kothari <faiz.off93@gmail.com>:
>>> From: Faiz Kotahri <faiz.off93@gmail.com>
>>>
>>> Signed-off-by: Faiz Kothari <faiz.off93@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 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 < state->nr_written; i++)
>>> free(state->written[i]);
>>>
>>> clear_exit:
>>> free(state->written);
>>> memset(state, 0, sizeof(*state));
>>> + strbuf_release(&packname);
>>>
>>> /* Make objects we just wrote available to ourselves */
>>> reprepare_packed_git();
>>> diff --git a/pack-write.c b/pack-write.c
>>> index 9b8308b..60f5734 100644
>>> --- a/pack-write.c
>>> +++ b/pack-write.c
>>> @@ -1,6 +1,7 @@
>>> #include "cache.h"
>>> #include "pack.h"
>>> #include "csum-file.h"
>>> +#include "strbuf.h"
>>>
>>> void reset_pack_idx_option(struct pack_idx_option *opts)
>>> {
>>> @@ -336,7 +337,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 +345,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 ini_length = name_buffer->len;
>>>
>>> if (adjust_shared_perm(pack_tmp_name))
>>> die_errno("unable to make temporary pack file readable");
>>> @@ -354,17 +355,17 @@ 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(end_of_name_prefix, "%s.idx", sha1_to_hex(sha1));
>>> - if (rename(idx_tmp_name, name_buffer))
>>> + strbuf_remove(name_buffer, ini_length, name_buffer->len - ini_length);
>>> + strbuf_addf(name_buffer, "%s.idx", sha1_to_hex(sha1));
>>> +
>>> + if (rename(idx_tmp_name, name_buffer->buf))
>>> die_errno("unable to rename temporary index file");
>>>
>>> - *end_of_name_prefix = '\0';
>>> -
>>> free((void *)idx_tmp_name);
>>> }
>>> diff --git a/pack.h b/pack.h
>>> index 12d9516..0afe8d1 100644
>>> --- a/pack.h
>>> +++ b/pack.h
>>> @@ -3,6 +3,7 @@
>>>
>>> #include "object.h"
>>> #include "csum-file.h"
>>> +#include "strbuf.h"
>>>
>>> /*
>>> * Packed object header
>>> @@ -91,6 +92,6 @@ extern int encode_in_pack_object_header(enum object_type, uintmax_t, unsigned ch
>>> extern int read_pack_header(int fd, struct pack_header *);
>>>
>>> extern struct sha1file *create_tmp_packfile(char **pack_tmp_name);
>>> -extern void finish_tmp_packfile(char *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, struct pack_idx_option *pack_idx_opts, unsigned char sha1[]);
>>> +extern void finish_tmp_packfile(struct strbuf *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, struct pack_idx_option *pack_idx_opts, unsigned char sha1[]);
>>>
>>> #endif
>>> --
>>> 1.7.9.5
>>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rewrite finish_bulk_checkin() using strbuf
2014-03-01 11:03 ` He Sun
@ 2014-03-01 11:20 ` Faiz Kothari
0 siblings, 0 replies; 5+ messages in thread
From: Faiz Kothari @ 2014-03-01 11:20 UTC (permalink / raw)
To: He Sun, git, Eric Sunshine
On Sat, Mar 1, 2014 at 4:33 PM, He Sun <sunheehnus@gmail.com> wrote:
> 2014-03-01 15:18 GMT+08:00 Faiz Kothari <faiz.off93@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 Sunshine<sunshine@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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-03-01 11:20 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-01 6:46 [PATCH] rewrite finish_bulk_checkin() using strbuf Faiz Kothari
2014-03-01 7:10 ` He Sun
2014-03-01 7:10 ` He Sun
[not found] ` <CAFbjVcmghtj80NetKoBeCKfMcECotiC=Q7ZmBR9ASuAtT_wiWg@mail.gmail.com>
2014-03-01 11:03 ` He Sun
2014-03-01 11:20 ` Faiz Kothari
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).