Re: [PATCH v2] finish_tmp_packfile():use strbuf for pathname construction
2014-03-03 15:41 GMT+08:00 Eric Sunshine sunsh...@sunshineco.com:
On Sat, Mar 1, 2014 at 9:29 PM, Sun He sunheeh...@gmail.com wrote:
Signed-off-by: Sun He sunheeh...@gmail.com
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Helped-by: Michael Haggerty mhag...@alum.mit.edu
---
This patch has assumed that you have already fix the bug of
tmpname in builtin/pack-objects.c:write_pack_file() warning()
builtin/pack-objects.c | 15 ++-
bulk-checkin.c | 8 +---
pack-write.c | 18 ++
pack.h | 2 +-
4 files changed, 22 insertions(+), 21 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..099d6ed 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
@@ -826,23 +826,19 @@ static void write_pack_file(void)
tmpname, 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);
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));
Transpose the space and comma before the third argument.
Other than this, the patch looks reasonable.
OK, got it.
I have already fixed it.
Thank you very much
stop_progress(progress_state);
@@ -851,10 +847,11 @@ 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..98e651c 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,8 +44,8 @@ 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++)
@@ -54,6 +55,7 @@ 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..9ccf804 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char
Re: [PATCH] Use ALLOC_GROW() instead of inline code
(array-nr + 1);
- array-items = xrealloc(array-items, array-alloc *
- sizeof(struct reflog_info));
- }
+ ALLOC_GROW(array-items, array-nr + 1, array-alloc);
item = array-items + array-nr;
memcpy(item-osha1, osha1, 20);
memcpy(item-nsha1, nsha1, 20);
@@ -114,11 +110,8 @@ static void add_commit_info(struct commit *commit, void
*util,
struct commit_info_lifo *lifo)
{
struct commit_info *info;
- if (lifo-nr = lifo-alloc) {
- lifo-alloc = alloc_nr(lifo-nr + 1);
- lifo-items = xrealloc(lifo-items,
- lifo-alloc * sizeof(struct commit_info));
- }
+
+ ALLOC_GROW(lifo-items, lifo-nr + 1, lifo-alloc);
info = lifo-items + lifo-nr;
info-commit = commit;
info-util = util;
diff --git a/replace_object.c b/replace_object.c
index cdcaf8c..843deef 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -36,12 +36,8 @@ static int register_replace_object(struct replace_object
*replace,
return 1;
}
pos = -pos - 1;
- if (replace_object_alloc = ++replace_object_nr) {
- replace_object_alloc = alloc_nr(replace_object_alloc);
- replace_object = xrealloc(replace_object,
- sizeof(*replace_object) *
- replace_object_alloc);
- }
+ ALLOC_GROW(replace_object, replace_object_nr + 1,
replace_object_alloc);
+ replace_object_nr++;
if (pos replace_object_nr)
memmove(replace_object + pos + 1,
replace_object + pos,
--
1.8.3.2
--
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
Cheers,
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 v2] Place cache.h at the first place to match general rule
2014-03-02 12:34 GMT+08:00 Eric Sunshine sunsh...@sunshineco.com:
On Sat, Mar 1, 2014 at 9:18 PM, Sun He sunheeh...@gmail.com wrote:
Signed-off-by: Sun He sunheeh...@gmail.com
Helped-by: Duy Nguyen pclo...@gmail.com
Footers should follow a temporal order. For instance:
1. Duy helped you.
2. You revised your patch based upon his input.
3. You signed off before submitting the patch.
Hence, your Signed-off-by: should follow Helped-by:.
OK, got it.
Thank you very much.
---
PATCH v2 Fix the spelling bug of general in subject as is suggested
by brain m.calson sand...@crustytoothpaste.net
There are two type of information you want to convey to readers:
1. Explanation and justification of the change itself. This is
recorded for all time in the project history as the commit message. It
is placed above the --- line.
2. Commentary related to this version / submission of the patch which
is not likely to be helpful or meaningful to people reading the
official project history via the commit messages. It is placed below
the --- line.
Explaining what you changed since the previous version of the patch,
as you do above, is a good thing. It's not meaningful once the patch
is officially accepted into the project; it's only meaningful to
people following the progression of the patch on the mailing list, so
it definitely belongs below the --- line, as you did here.
However...
The general rule is if cache.h or git-compat-util.h is included,
it is the first #include.
This information explains the patch's purpose, thus it is relevant to
the project history. It belongs above the --- line.
OK, Got it. I will move above the --- line soon, and I will pay attention
not to make mistakes like this from now on.
Thank you very much.
I parsed all the source files, and find many files start with builtin.h.
And git-compat-util.h is the first in it. So they don't need any change.
This could go either way. It tells how you arrived at this version of
the patch (relevant below ---), but also explains why the patch does
not have to touch additional files (relevant above ---). It's
probably okay to leave it below ---.
sigchain.c and test-sigchain.c are started with sigchain.h
I checked sigchain.h, and it didn't import any bug.
But to keep consistant with general rule, we should take this patch.
Commentary suitable for below ---.
Thanks.
sigchain.c | 2 +-
test-sigchain.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sigchain.c b/sigchain.c
index 1118b99..faa375d 100644
--- a/sigchain.c
+++ b/sigchain.c
@@ -1,5 +1,5 @@
-#include sigchain.h
#include cache.h
+#include sigchain.h
#define SIGCHAIN_MAX_SIGNALS 32
diff --git a/test-sigchain.c b/test-sigchain.c
index 42db234..e499fce 100644
--- a/test-sigchain.c
+++ b/test-sigchain.c
@@ -1,5 +1,5 @@
-#include sigchain.h
#include cache.h
+#include sigchain.h
#define X(f) \
static void f(int sig) { \
--
1.9.0.138.g2de3478.dirty
I am honored to get detailed guidance from you.
Thank you very very much.
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 v3] write_pack_file: use correct variable in diagnostic
2014-03-03 2:42 GMT+08:00 Eric Sunshine sunsh...@sunshineco.com: On Sun, Mar 2, 2014 at 2:30 AM, Sun He sunheeh...@gmail.com wrote: 'pack_tmp_name' is the subject of the utime() check, so report it in the warning, not the uninitialized 'tmpname' Signed-off-by: Sun He sunheeh...@gmail.com --- Changing the subject and adding valid information as tutored by Eric Sunshine. Thanks to him. builtin/pack-objects.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index c733379..4922ce5 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -823,7 +823,7 @@ 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? */ -- 1.9.0.138.g2de3478.dirty Nicely done. Everything is where it ought to be. As this is an actual bug fix (not just a GSoC microproject): Reviewed-by: Eric Sunshine sunsh...@sunshineco.com Thank you so much for everything you have done. I am honored to do this with all your support and kindness. Regards， 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 v3] branch.c: change install_branch_config() to use skip_prefix()
2014-03-03 10:24 GMT+08:00 Guanglin Xu mzguang...@gmail.com:
to avoid a magic code of 11.
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Helped-by: Jacopo Notarstefano jaco...@gmail.com
Signed-off-by: Guanglin Xu mzguang...@gmail.com
---
This is an implementation of the idea#2 of GSoC 2014 microproject.
branch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/branch.c b/branch.c
index 723a36b..3e2551e 100644
--- a/branch.c
+++ b/branch.c
@@ -49,7 +49,7 @@ static int should_setup_rebase(const char *origin)
void install_branch_config(int flag, const char *local, const char *origin,
const char *remote)
{
- const char *shortname = remote + 11;
+ const char *shortname = skip_prefix(remote ,refs/heads/);
Maybe it is more proper to avoid the test of remote_is_branch, by
testing shortname instead.
And add the comment skip_prefix only return NULL when refs/heads/ is
not the prefix of remote
int remote_is_branch = starts_with(remote, refs/heads/);
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
--
1.9.0
--
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
Cheers,
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 v3] branch.c: change install_branch_config() to use skip_prefix()
2014-03-03 10:24 GMT+08:00 Guanglin Xu mzguang...@gmail.com:
to avoid a magic code of 11.
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Helped-by: Jacopo Notarstefano jaco...@gmail.com
Signed-off-by: Guanglin Xu mzguang...@gmail.com
---
This is an implementation of the idea#2 of GSoC 2014 microproject.
branch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/branch.c b/branch.c
index 723a36b..3e2551e 100644
--- a/branch.c
+++ b/branch.c
@@ -49,7 +49,7 @@ static int should_setup_rebase(const char *origin)
void install_branch_config(int flag, const char *local, const char *origin,
const char *remote)
{
- const char *shortname = remote + 11;
+ const char *shortname = skip_prefix(remote ,refs/heads/);
int remote_is_branch = starts_with(remote, refs/heads/);
Or it may be better to keep remote_is_branch, and replace starts_with
with something
you have just fixed.
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
--
1.9.0
--
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
--
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 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] implemented strbuf_write_or_die()
2014-03-01 19:21 GMT+08:00 Faiz Kothari faiz.of...@gmail.com:
Signed-off-by: Faiz Kothari faiz.of...@gmail.com
---
Implemented write_or_die.c:strbuf_write_or_die() and used in relevant places
to substitute write_or_die(). I spotted other places where strbuf can be used
in place of buf[MAX_PATH] but that would require a change in prototype of a
lot of functions and functions calling them and so on
I'll look for more places where strbuf can be used safely.
Thanks.
builtin/cat-file.c |2 +-
builtin/notes.c|4 ++--
builtin/receive-pack.c |2 +-
builtin/send-pack.c|2 +-
builtin/stripspace.c |2 +-
builtin/tag.c |2 +-
bundle.c |2 +-
cache.h|1 +
credential-store.c |2 +-
fetch-pack.c |2 +-
http-backend.c |2 +-
remote-curl.c |8 +---
write_or_die.c |9 +
13 files changed, 26 insertions(+), 14 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index d5a93e0..c756cd5 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -255,7 +255,7 @@ static int batch_one_object(const char *obj_name, struct
batch_options *opt,
strbuf_expand(buf, opt-format, expand_format, data);
strbuf_addch(buf, '\n');
- write_or_die(1, buf.buf, buf.len);
+ strbuf_write_or_die(1, buf);
strbuf_release(buf);
if (opt-print_contents) {
diff --git a/builtin/notes.c b/builtin/notes.c
index 2b24d05..ef40183 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -140,7 +140,7 @@ static void write_commented_object(int fd, const unsigned
char *object)
if (strbuf_read(buf, show.out, 0) 0)
die_errno(_(could not read 'show' output));
strbuf_add_commented_lines(cbuf, buf.buf, buf.len);
- write_or_die(fd, cbuf.buf, cbuf.len);
+ strbuf_write_or_die(fd, cbuf);
strbuf_release(cbuf);
strbuf_release(buf);
@@ -174,7 +174,7 @@ static void create_note(const unsigned char *object,
struct msg_arg *msg,
strbuf_addch(buf, '\n');
strbuf_add_commented_lines(buf, note_template,
strlen(note_template));
strbuf_addch(buf, '\n');
- write_or_die(fd, buf.buf, buf.len);
+ strbuf_write_or_die(fd, buf);
write_commented_object(fd, object);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 85bba35..9434516 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1114,7 +1114,7 @@ static void report(struct command *commands, const char
*unpack_status)
if (use_sideband)
send_sideband(1, 1, buf.buf, buf.len, use_sideband);
else
- write_or_die(1, buf.buf, buf.len);
+ strbuf_write_or_die(1, buf);
strbuf_release(buf);
}
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f420b74..d053f0a 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -86,7 +86,7 @@ static void print_helper_status(struct ref *ref)
}
strbuf_addch(buf, '\n');
- write_or_die(1, buf.buf, buf.len);
+ strbuf_write_or_die(1, buf);
}
strbuf_release(buf);
}
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 1259ed7..cf5c876 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -115,7 +115,7 @@ int cmd_stripspace(int argc, const char **argv, const
char *prefix)
else
comment_lines(buf);
- write_or_die(1, buf.buf, buf.len);
+ strbuf_write_or_die(1, buf);
strbuf_release(buf);
return 0;
}
diff --git a/builtin/tag.c b/builtin/tag.c
index 74d3780..5af6ea3 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -349,7 +349,7 @@ static void create_tag(const unsigned char *object, const
char *tag,
strbuf_commented_addf(buf, _(tag_template),
comment_line_char);
else
strbuf_commented_addf(buf,
_(tag_template_nocleanup), comment_line_char);
- write_or_die(fd, buf.buf, buf.len);
+ strbuf_write_or_die(fd, buf);
strbuf_release(buf);
}
close(fd);
diff --git a/bundle.c b/bundle.c
index e99065c..435505d 100644
--- a/bundle.c
+++ b/bundle.c
@@ -279,7 +279,7 @@ int create_bundle(struct bundle_header *header, const
char *path,
while (strbuf_getwholeline(buf, rls_fout, '\n') != EOF) {
unsigned char sha1[20];
if (buf.len 0 buf.buf[0] == '-') {
- write_or_die(bundle_fd, buf.buf, buf.len);
+ strbuf_write_or_die(bundle_fd, buf);
if (!get_sha1_hex(buf.buf + 1, sha1)) {
Re: [PATCH] Replace memcpy with hashcpy when dealing hash copy globally
2014-03-01 10:58 GMT+08:00 Duy Nguyen pclo...@gmail.com: On Sat, Mar 1, 2014 at 8:07 AM, Sun He sunheeh...@gmail.com wrote: Signed-off-by: Sun He sunheeh...@gmail.com --- Find the potential places with memcpy by the bash command: $ find . | xargs grep memcpy.*\(.*20.*\) Helped-by: Michael Haggertymhag...@alum.mit.edu You may want to put this Helped-by before --- because it's supposed to end up in the final commit. The patch looks straightforward, except.. Yeah, got it. Thanks. diff --git a/ppc/sha1.c b/ppc/sha1.c index ec6a192..8a87fea 100644 --- a/ppc/sha1.c +++ b/ppc/sha1.c @@ -9,6 +9,7 @@ #include stdio.h #include string.h #include sha1.h +#include cache.h extern void ppc_sha1_core(uint32_t *hash, const unsigned char *p, unsigned int nblocks); @@ -67,6 +68,6 @@ int ppc_SHA1_Final(unsigned char *hash, ppc_SHA_CTX *c) memset(c-buf.b[cnt], 0, 56 - cnt); c-buf.l[7] = c-len; ppc_sha1_core(c-hash, c-buf.b, 1); - memcpy(hash, c-hash, 20); + hashcpy(hash, c-hash); return 0; } cache.h (actually git-compat-util.h that cache.h includes) messes around with system headers by defining this and that macro. The general rule is if cache.h or git-compat-util.h is included, it's the first #include, and system includes will be always in git-compat-util.h (grep '^#include' shows this). Maybe it's best to leave this memcpy alone (and if you do, state so in the commit message with the reason). Yap, after I parsed all the sourcecode I have find out all the files that cache.h git-compat-util.h and builtin.h are not the first #include == test-sigchain.c == #include sigchain.h == sigchain.c == #include sigchain.h And I checked sigchain.h, that it includes very little information. It didn't import any potential errors. But I think it should be placed after cache.h to match the consistence of the general rule. -- Duy -- 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] Replace memcpy with hashcpy when dealing hash copy globally
2014-03-01 10:58 GMT+08:00 Duy Nguyen pclo...@gmail.com: On Sat, Mar 1, 2014 at 8:07 AM, Sun He sunheeh...@gmail.com wrote: Signed-off-by: Sun He sunheeh...@gmail.com --- Find the potential places with memcpy by the bash command: $ find . | xargs grep memcpy.*\(.*20.*\) Helped-by: Michael Haggertymhag...@alum.mit.edu You may want to put this Helped-by before --- because it's supposed to end up in the final commit. The patch looks straightforward, except.. Yeah, got it. Thanks. diff --git a/ppc/sha1.c b/ppc/sha1.c index ec6a192..8a87fea 100644 --- a/ppc/sha1.c +++ b/ppc/sha1.c @@ -9,6 +9,7 @@ #include stdio.h #include string.h #include sha1.h +#include cache.h extern void ppc_sha1_core(uint32_t *hash, const unsigned char *p, unsigned int nblocks); @@ -67,6 +68,6 @@ int ppc_SHA1_Final(unsigned char *hash, ppc_SHA_CTX *c) memset(c-buf.b[cnt], 0, 56 - cnt); c-buf.l[7] = c-len; ppc_sha1_core(c-hash, c-buf.b, 1); - memcpy(hash, c-hash, 20); + hashcpy(hash, c-hash); return 0; } cache.h (actually git-compat-util.h that cache.h includes) messes around with system headers by defining this and that macro. The general rule is if cache.h or git-compat-util.h is included, it's the first #include, and system includes will be always in git-compat-util.h (grep '^#include' shows this). Maybe it's best to leave this memcpy alone (and if you do, state so in the commit message with the reason). Yap, I should follow the general rule. My fault. Thanks. What's more, I have find out all the files that cache.h git-compat-util.h and builtin.h are not the first #include == test-sigchain.c == #include sigchain.h == sigchain.c == #include sigchain.h And I checked sigchain.h, that it includes very little information. It didn't import any potential errors. But I think it should be placed after cache.h to match the consistence of the general rule. -- Duy -- 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] Replace memcpy with hashcpy when dealing hash copy globally
Got it. Thanks. 2014-03-01 17:13 GMT+08:00 Tay Ray Chuan rcta...@gmail.com: On Sat, Mar 1, 2014 at 10:58 AM, Duy Nguyen pclo...@gmail.com wrote: On Sat, Mar 1, 2014 at 8:07 AM, Sun He sunheeh...@gmail.com wrote: Signed-off-by: Sun He sunheeh...@gmail.com --- Find the potential places with memcpy by the bash command: $ find . | xargs grep memcpy.*\(.*20.*\) Helped-by: Michael Haggertymhag...@alum.mit.edu You may want to put this Helped-by before --- because it's supposed to end up in the final commit. To elaborate further: anything below the three-dash is ignored when the patch is applied. So it doesn't show up in the commit at all. I don't really know the reason for this - probably a patch (1) thing. You could put the patch the Helped-by before your Signed-off-by (sometimes referred to as S-o-b). -- Cheers, Ray Chuan -- 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 v2] Replace tmpname with pack_tmp_name in warning. The developer mistook tmpname for pack_tmp_name.
2014-03-02 11:59 GMT+08:00 Eric Sunshine sunsh...@sunshineco.com: On Sat, Mar 1, 2014 at 9:43 PM, Sun He sunheeh...@gmail.com wrote: Subject: Replace tmpname with pack_tmp_name in warning. The developer mistook tmpname for pack_tmp_name. The subject should be a short summary of the change, and the rest of the commit message before the --- line provides extra detail explaining the change. OK, got it. Thank you very much. Signed-off-by: Sun He sunheeh...@gmail.com --- As tmpname is used without initialization, it should be a mistake. This is valid information for the commit message above the --- line. So, your full commit message might say something like this: Subject: write_pack_file: use correct variable in diagnostic 'pack_tmp_name' is the subject of the utime() check, so report it in the warning, not the uninitialized 'tmpname'. Thank you for your suggestion, I will modify it right now. :-) builtin/pack-objects.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index c733379..4922ce5 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -823,7 +823,7 @@ 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? */ -- 1.9.0.138.g2de3478.dirty -- 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] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE
2014-03-01 3:42 GMT+08:00 Junio C Hamano gits...@pobox.com: Michael Haggerty mhag...@alum.mit.edu writes: On 02/28/2014 10:07 AM, Sun He wrote: 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 Hi, When I was reading code yesterday, I came across this protential bug. parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option. Please, no overlong line in the text part that makes things unnecessarily hard to read. I agree with all the comments in the message I am responding to. OK, got it. Thanks. BTW, none of my comments should be taken to indicate whether the commit itself makes sense or not. I haven't checked that far. While I think it is sensible to make sure CMDMODE is not marked to allow arguments, I do not understand why special type would imply it is allowed to be marked to take an argument. Why is it a good change to remove case OPTION_NUMBER: here? The comments in parse-options.h (Line 10-16) says that CMDMODE is not marked to allow arguments. And I am not sure if removing OPTION_NUMBER is a proper way. -- 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-03-01 4:02 GMT+08:00 Junio C Hamano [via git] ml-node+s661346n7604560...@n2.nabble.com: Duy Nguyen [hidden email] writes: 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. Also, I do not think this should be done in a single patch. What if it turns out that explicitly making sure that CMDMODE does not take any argument is a good idea, but the other change is a bad one? OK, I will try to figure it out. -- 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-tp7604513p7604560.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
Re: [PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname
2014-03-01 4:42 GMT+08:00 Eric Sunshine sunsh...@sunshineco.com: On Fri, Feb 28, 2014 at 9:17 AM, 孙赫 sunheeh...@gmail.com wrote: 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(). I'm not sure what you mean by checking the string length before sprintf(). That's because one is not certain of the length of get_object_directory() Micheal Mhaggertymhag...@alum.mit.edu explained this to me. Saving stack space is nice, though given that it takes more time to allocate space on the heap, it is nonetheless usually preferred to use the stack for temporary variables of this size. The problem has more to do with the fact that the old version fixes a maximum length on the buffer, which could be a problem if one is not certain of the length of get_object_directory(). The other point of strbuf is that you don't have to do the error-prone bookkeeping yourself. So it would be preferable to use strbuf_addf(). It is the same as yours about the space and time costs. ^_^ 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. OK, OK, Got it. Thank you very much. Cheers, 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] rewrite bulk-checkin.c:finish_bulk_checkin() using strbuf
); for (i = 0; i state-nr_written; i++) diff --git a/pack-write.c b/pack-write.c index 605d01b..ac38867 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(const char *name_buffer, This is misleading and fragile. By specifying 'const', finish_tmp_packfile() promises not to modify the content of the incoming name_buffer, yet it breaks this promise by modifying the buffer through the non-const end_of_name_prefix variable (after dropping the 'const' via strrchr()). const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, -- 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 Cheers, 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] rewrite finish_bulk_checkin() using strbuf
;
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
--
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.
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
