Re: [PATCH] Setup.c: PATH_MAX is the length including the Nil

2014-03-04 Thread Faiz Kothari 
On Tue, Mar 4, 2014 at 9:59 PM, Sun He sunheeh...@gmail.com wrote:
 Signed-off-by: Sun He sunheeh...@gmail.com
 ---

 Check the limit.h of linux and find out that the MACRO
 #define PATH_MAX4096/* # chars in a path name including nul */
 So if the magic number 40 is just the size it should be. (e.g. hash code)
 It may bring bugs with the length(4056) of long name(gitdirenv).
 As gitdirenv could be set by GIT_DIR_ENVIRONMENT.
 If it is a bug, it will almost never occur.
 But I need your help to know if there is the PATH_MAX of git is the mirror of 
 the
 PATH_MAX of linux and if this fix is right?
 If it was, there may be many places like PATH_MAX + 1 could be replaced by
 just PATH_MAX. And there may be many places like this.

 Cheers,
 He Sun

Hi,
I am not getting what exactly you are trying to tell, but git defines
its own PATH_MAX.
Its defined in git-compat-util.h: #define PATH_MAX 4096
That is why instead of making buffers using PATH_MAX, use strbuf.
All these problems of buffer overflow will be gone.
I hope you are concerned about buffer overflow.

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] implemented strbuf_write_or_die()

2014-03-04 Thread Faiz Kothari 
 I'm the guilty one.  I like the change (obviously, since I suggested
 it).  Writing strbufs comes up frequently and will hopefully increase in
 usage and I think it is a positive thing to encourage the use of strbufs
 by making them increasingly first-class citizens.

 But I can see your points too, and I humbly defer to the wisdom of the
 list.  I will remove this suggestion from the list of microprojects.

 Faiz, this is the way things go on the Git mailing list.  It would be
 boring if everybody agreed all the time :-)

 Michael

Hi,
Thank you all. Even I like the strbuf_write_or_die() but again its a
code churn as pointed out. But if we want to use strbuf instead of
static buffers we might need this function very often (Its just my
opinion).
Anyways, implementing it was an exercise and I enjoyed it. I agree
with Michael Haggerty that it would be boring if everybody agreed all
the time :D
I enjoyed it and learnt from the exercise, so I don't think it was a
waste or a bad exercise. At least it exposed me to practices of good
software design and importance of layers in software.

Thanks a lot.

-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

Fwd: [PATCH] implemented strbuf_write_or_die()

2014-03-03 Thread Faiz Kothari 
On Tue, Mar 4, 2014 at 12:01 AM, Junio C Hamano gits...@pobox.com wrote:
 Eric Sunshine sunsh...@sunshineco.com writes:

 On Sat, Mar 1, 2014 at 7:51 AM, He Sun sunheeh...@gmail.com wrote:
 2014-03-01 19:21 GMT+08:00 Faiz Kothari faiz.of...@gmail.com:
 diff --git a/remote-curl.c b/remote-curl.c
 index 10cb011..dee8716 100644
 --- a/remote-curl.c
 +++ b/remote-curl.c
 @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct 
 discovery *heads)
 if (start_command(client))
 exit(1);
 if (preamble)
 -   write_or_die(client.in, preamble-buf, preamble-len);
 +   strbuf_write_or_die(client.in, preamble);
 if (heads)
 write_or_die(client.in, heads-buf, heads-len);

 This should be changed. May be you can use Ctrl-F to search write_or_die().
 Or if you are using vim, use / and n to find all.

 It's not obvious from the patch fragment, but 'heads' is not a strbuf,
 so Faiz correctly left this invocation alone.

 That is a very good sign why this change is merely a code-churn and
 not an improvement, isn't it?  We know (and any strbuf user should
 know) that -buf and -len are the ways to learn the pointer and the
 length the strbuf holds.  Why anybody thinks it is benefitial to
 introduce another function that is _only_ for writing out strbuf and
 cannot be used to write out a plain buffer is simply beyond me.

Hi,
Thanks for the feedback. Yes, I do realize, its kind of a code churn.
I didn't realize it until I looked at the sign you pointed out.
But it was a good exercise to go through the code as this is one of
the GSoC microprojects.
Sorry, it didn't turn out to be a beneficial one. My bad.

Thanks a lot again for the suggestions and feedback.

-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 Thread Faiz Kothari 
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] implemented strbuf_write_or_die()

2014-03-01 Thread Faiz Kothari 
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)) {
struct object *object = 
parse_object_or_die(sha1, buf.buf);
object-flags |= UNINTERESTING;
diff --git a/cache.h b/cache.h
index

[PATCH] implemented strbuf_write_or_die()

2014-03-01 Thread Faiz Kothari 
Signed-off-by: Faiz Kothari faiz.of...@gmail.com
---
  -   write_or_die(1, rpc.result.buf, rpc.result.len);
  +   strbuf_write_or_die(1, (rpc.result.buf));

 May be this should be
 strbuf_write_or_die(1, (rpc.result));

Yes, I changed that :-) Thanks again.

 Maybe we just call write_or_die() in strbuf_write_or_die(), in case that if we
 wanna change something in write_or_dir(), we don't have to do duplicate jobs.

Yes I changed it. It was unnecessary to reimplement it.

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 |6 ++
 13 files changed, 23 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)) {
struct object *object = 
parse_object_or_die(sha1, buf.buf

[PATCH v2] implemented strbuf_write_or_die()

2014-03-01 Thread Faiz Kothari 
Signed-off-by: Faiz Kothari faiz.of...@gmail.com
---
Thanks for the feedback.
Implemented write_or_dir.c:strbuf_write_or_die() again.
Checks if NULL is passed to prevent segmentation fault, I was not sure 
what error message to print so for now its write error.
Changed the prototype as suggested.
Implementing this clearly distinguishes between writing a normal buffer
and writing a strbuf. Also, it provides an interface to write strbuf
directly without knowing the private members of strbuf, making strbuf 
completely opaque. Also, makes the code more readable.
I hope its proper now.

Thanks.

 builtin/cat-file.c |2 +-
 builtin/notes.c|6 +++---
 builtin/receive-pack.c |2 +-
 builtin/send-pack.c|2 +-
 builtin/stripspace.c   |2 +-
 builtin/tag.c  |2 +-
 bundle.c   |2 +-
 cache.h|1 +
 fetch-pack.c   |2 +-
 http-backend.c |2 +-
 remote-curl.c  |6 +++---
 write_or_die.c |   10 ++
 12 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index d5a93e0..d07a0be 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(buf, 1);
strbuf_release(buf);
 
if (opt-print_contents) {
diff --git a/builtin/notes.c b/builtin/notes.c
index 2b24d05..a208d56 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(cbuf, fd);
 
strbuf_release(cbuf);
strbuf_release(buf);
@@ -167,14 +167,14 @@ static void create_note(const unsigned char *object, 
struct msg_arg *msg,
die_errno(_(could not create file '%s'), path);
 
if (msg-given)
-   write_or_die(fd, msg-buf.buf, msg-buf.len);
+   strbuf_write_or_die((msg-buf), fd);
else if (prev  !append_only)
write_note_data(fd, prev);
 
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(buf, fd);
 
write_commented_object(fd, object);
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 85bba35..d590993 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(buf, 1);
strbuf_release(buf);
 }
 
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f420b74..f26ba21 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(buf, 1);
}
strbuf_release(buf);
 }
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 1259ed7..33b7f85 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(buf, 1);
strbuf_release(buf);
return 0;
 }
diff --git a/builtin/tag.c b/builtin/tag.c
index 74d3780..53ab280 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(buf, fd);
strbuf_release(buf);
}
close(fd);
diff --git a/bundle.c b/bundle.c
index e99065c..c8bddd8 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

[PATCH v3 1/2] Introduce strbuf_write_or_die()

2014-03-01 Thread Faiz Kothari 
Signed-off-by: Faiz Kothari faiz.of...@gmail.com

Introduced a new function strbuf.c:strbuf_write_or_die()
to the strbuf family of functions. Now use this API instead
of write_or_die.c:write_or_die()
---
Hi,
Thanks for the suggestions and feedbacks.
As Johannes Sixt  pointed out, the function is now defined
in strbuf.c and prototype added to strbuf.h
Also, replaced if(!sbuf) with assert(sbuf) and split the patch into two 
as pointed out by Eric Sunshine.

As far as justification is concerned, I am not able to come up with
a satisfactory justification. Apart from, that it makes life of the
programmer a little easier and if we add a few more functions
to thestrbuf API, we can make strbuf completely opaque. I am open
to views and since I haven't used this API extensively, I cannot
comment for what is missing and what is required. But I am going through it.
Also, once this patch is OK, I'll add documentation for the API.

Thanks again for the feedback.

 strbuf.c |6 ++
 strbuf.h |1 +
 2 files changed, 7 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index 83caf4a..337a70c 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -477,6 +477,12 @@ int strbuf_read_file(struct strbuf *sb, const char *path, 
size_t hint)
return len;
 }
 
+void strbuf_write_or_die(const struct strbuf *sb, int fd)
+{
+   assert(sb);
+   write_or_die(fd, sb-buf, sb-len);
+}
+
 void strbuf_add_lines(struct strbuf *out, const char *prefix,
  const char *buf, size_t size)
 {
diff --git a/strbuf.h b/strbuf.h
index 73e80ce..6aadb6d 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -156,6 +156,7 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
 /* XXX: if read fails, any partial read is undone */
 extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
 extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);
+extern void strbuf_write_or_die(const struct strbuf *sb, int fd);
 extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
 
 extern int strbuf_getwholeline(struct strbuf *, FILE *, int);
-- 
1.7.9.5

--
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 v3 2/2] use strbuf_write_or_die()

2014-03-01 Thread Faiz Kothari 
Signed-off-by: Faiz Kothari faiz.of...@gmail.com

Used strbuf.c:strbuf_write_or_die() instead of
write_or_die.c:write_or_die() at relevant places.
---
 builtin/cat-file.c |2 +-
 builtin/notes.c|6 +++---
 builtin/receive-pack.c |2 +-
 builtin/send-pack.c|2 +-
 builtin/stripspace.c   |2 +-
 builtin/tag.c  |2 +-
 bundle.c   |2 +-
 credential-store.c |2 +-
 fetch-pack.c   |2 +-
 http-backend.c |2 +-
 remote-curl.c  |6 +++---
 11 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index d5a93e0..d07a0be 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(buf, 1);
strbuf_release(buf);
 
if (opt-print_contents) {
diff --git a/builtin/notes.c b/builtin/notes.c
index 2b24d05..a208d56 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(cbuf, fd);
 
strbuf_release(cbuf);
strbuf_release(buf);
@@ -167,14 +167,14 @@ static void create_note(const unsigned char *object, 
struct msg_arg *msg,
die_errno(_(could not create file '%s'), path);
 
if (msg-given)
-   write_or_die(fd, msg-buf.buf, msg-buf.len);
+   strbuf_write_or_die((msg-buf), fd);
else if (prev  !append_only)
write_note_data(fd, prev);
 
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(buf, fd);
 
write_commented_object(fd, object);
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 85bba35..d590993 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(buf, 1);
strbuf_release(buf);
 }
 
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f420b74..f26ba21 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(buf, 1);
}
strbuf_release(buf);
 }
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 1259ed7..33b7f85 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(buf, 1);
strbuf_release(buf);
return 0;
 }
diff --git a/builtin/tag.c b/builtin/tag.c
index 74d3780..53ab280 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(buf, fd);
strbuf_release(buf);
}
close(fd);
diff --git a/bundle.c b/bundle.c
index e99065c..c8bddd8 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(buf, bundle_fd);
if (!get_sha1_hex(buf.buf + 1, sha1)) {
struct object *object = 
parse_object_or_die(sha1, buf.buf);
object-flags |= UNINTERESTING;
diff --git a/credential-store.c b

Re: [PATCH] rewrite bulk-checkin.c:finish_bulk_checkin() using strbuf

2014-02-28 Thread Faiz Kothari 
Hi,
Thanks for the suggestions and remarks.
I rewrote bulk-checkin.c:finish_bulk_checkin() using strbuf. But saw
that Sun He has already implemented the same way I have done.
Should I submit my implementation as a patch?

Secondly,
I tried implementing this WITHOUT changing the prototype of the
function pack-write.c:finish_tmp_packfile().

For this I detached the buffer from strbuf in finish_bulk_checkin()
using strbuf_detach() and passed it to finish_tmp_packfile().

Inside finish_tmp_packfile, I attached the same buffer to a local
struct strbuf using strbuf_attach().
Now the problem is, two of the arguments to strbuf_attach() are
'alloc' and 'len' which are private members of the struct strbuf.
But since I am just passing the detached buffer, the information of
alloc and len is lost which is required at the time of attaching.
I cannot think of any better way of using strbuf and NOT modify the
prototype of finish_tmp_packfile()

As a workaround, I can determine alloc = (strlen(buf) + 1) and len =
strlen(buf) but AFAIK this is not always true and may break.
Any suggestions?

Thanks.

On Fri, Feb 28, 2014 at 2:45 PM, Eric Sunshine sunsh...@sunshineco.com wrote:
 On Fri, Feb 28, 2014 at 2:58 AM, Faiz Kothari faiz.of...@gmail.com wrote:
 Signed-off-by: Faiz Kothari faiz.of...@gmail.com

 Notes:
 I finally got what's happening, and why the errors were caused.
 packname is supposed to contain the complete path to the .pack file.
 Packs are stored as /path/to/SHA1.pack which I overlooked earlier.
 After inspecting what is happening in pack-write.c:finish_tmp_packfile()
 which indirectly modifies packname by appending the SHA1 and .pack to 
 packname
 This is happening in these code snippets:
 char *end_of_name_prefix = strrchr(name_buffer, 0);

 and later
 sprintf(end_of_name_prefix, %s.pack, sha1_to_hex(sha1));

 name_buffer is packname.buf
 Using const for the first argument of pack-write.c:finish_tmp_packfile()
 doesnot raise any compile time warning or error and not any runtime 
 errors,
 since the packname.buf is on heap and has extra space to which more char 
 can be written.
 If this was not the case,
 for e.g. passing a constant string and modifying it.
 This will result in a segmentation fault.
 ---

 This notes section is important to the ongoing email discussion,
 however, it should be placed below the --- line so that it does not
 become part of the recorded commit message when the patch is applied
 via git am.

  bulk-checkin.c |8 +---
  pack-write.c   |2 +-
  pack.h |2 +-
  3 files changed, 7 insertions(+), 5 deletions(-)

 diff --git a/bulk-checkin.c b/bulk-checkin.c
 index 118c625..bbdf1ec 100644
 --- a/bulk-checkin.c
 +++ b/bulk-checkin.c
 @@ -23,7 +23,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)
 @@ -42,9 +42,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
 *state)
  state-offset);
 close(fd);
 }
 +   strbuf_addf(packname, %s/pack/pack-, get_object_directory());
 +   strbuf_grow(packname, 40 + 5);

 There are several problems with this. First, magic numbers 40 and 5
 convey no meaning to the reader. At the very least, they should be
 named constants or a comment should explain them. More seriously,
 though, this code is fragile since it has far too intimate knowledge
 of the inner workings of finish_tmp_packfile(). If the implementation
 of finish_tmp_packfile() changes in the future such that it writes
 more than 45 additional characters to the incoming buffer, this will
 break.

 Rather than coupling finish_bulk_checkin() and finish_tmp_packfile()
 so tightly, consider finish_tmp_packfile() a black box which just
 does its job and then propose ways to make things work without
 finish_bulk_checkin() having to know how that job is done.

 -   sprintf(packname, %s/pack/pack-, get_object_directory());
 -   finish_tmp_packfile(packname, state-pack_tmp_name,
 +   finish_tmp_packfile(packname.buf, state-pack_tmp_name,
 state-written, state-nr_written,
 state-pack_idx_opts, sha1);
 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

[PATCH] rewrite finish_bulk_checkin() using strbuf

2014-02-28 Thread Faiz Kothari 
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] GSoC 2014 Microproject 1 rewrite skip_prefix() as loop

2014-02-27 Thread Faiz Kothari 
Thanks for the reply,
I was unable to get git send-email working. Now its working, I'll
resend the patch.
I ran all the tests, they are working properly.
About the comment, I meant, there is a similar function
strbuf.c:starts_with() which does the exact same job, but it returns 0
or 1.
I just changed it to return a (const char *) accordingly.

On Thu, Feb 27, 2014 at 5:02 PM, Michael Haggerty mhag...@alum.mit.edu wrote:
 On 02/26/2014 05:46 PM, Faiz Kothari wrote:
 I am Faiz Kothari, I am a GSoC aspirant and want to contribute to git.
 I am submitting the patch in reponse to Microproject 1,
 rewrite git-compat-util.h:skip_prefix() as a loop.

 Signed-off-by: Faiz Kothari faiz.of...@gmail.com

 The subject of your email plus the part above the --- line will be
 taken directly to be used as the commit message.  So it should not
 include information that is inappropriate for a commit message.

 You can put such information directly below the --- line.

 Please also see my comments below.

 ---
  git-compat-util.h | 7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)

 diff --git a/git-compat-util.h b/git-compat-util.h
 index cbd86c3..bb2582a 100644
 --- a/git-compat-util.h
 +++ b/git-compat-util.h
 @@ -357,8 +357,11 @@ extern int suffixcmp(const char *str, const char
 *suffix);

  static inline const char *skip_prefix(const char *str, const char
 *prefix)

 The line above seems to have been broken by your email program.  It is
 important for efficiency reasons that patches be readable directly out
 of emails (e.g., by using git am).  Please practice by sending the
 patch to yourself different ways until git am works on it correctly.

  {
 - size_t len = strlen(prefix);
 - return strncmp(str, prefix, len) ? NULL : str + len;
 + for (; ; str++, prefix++)
 + if (!*prefix)
 + return str;//code same as strbuf.c:starts_with()

 We don't use // for comments, and please space things out the way
 other code does it.  But actually, IMO this particular comment doesn't
 really belong permanently in the code.  It rather belongs in the commit
 message, or in the discussion (under the ---), or maybe it should be
 taken as an indication of a deeper problem (see below).

 + else if (*str != *prefix)
 + return NULL;
  }

  #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)


 The code itself looks correct.

 But, considering your comment, would it be appropriate for one of the
 functions to call the other?

 Michael

 --
 Michael Haggerty
 mhag...@alum.mit.edu
 http://softwareswirl.blogspot.com/
--
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 skip_prefix() as loop

2014-02-27 Thread Faiz Kothari 
From: Faiz Kothari faiz.of...@gmail.com


Signed-off-by: Faiz Kothari django@dj-pc.(none)
---
 git-compat-util.h |7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index cbd86c3..bb2582a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -357,8 +357,11 @@ extern int suffixcmp(const char *str, const char *suffix);
 
 static inline const char *skip_prefix(const char *str, const char *prefix)
 {
-   size_t len = strlen(prefix);
-   return strncmp(str, prefix, len) ? NULL : str + len;
+   for (; ; str++, prefix++)
+   if (!*prefix)
+   return str;//code same as strbuf.c:starts_with()
+   else if (*str != *prefix)
+   return NULL;
 }
 
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
-- 
1.7.9.5

--
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] GSoC2014 Microproject rewrite finish_bulk_checkin()

2014-02-27 Thread Faiz Kothari 
Signed-off-by: Faiz Kothari faiz.of...@gmail.com
---
 bulk-checkin.c |   12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 118c625..feeff9f 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -23,7 +23,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;
int i;
 
if (!state-f)
@@ -42,9 +42,11 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
*state)
 state-offset);
close(fd);
}
-
-   sprintf(packname, %s/pack/pack-, get_object_directory());
-   finish_tmp_packfile(packname, state-pack_tmp_name,
+   
+   packname.len = packname.alloc = 64 + strlen(get_object_directory());
+   packname.buf = (char *)malloc(packname.len * sizeof(char));
+   sprintf(packname.buf, %s/pack/pack-, get_object_directory());
+   finish_tmp_packfile(packname.buf, state-pack_tmp_name,
state-written, state-nr_written,
state-pack_idx_opts, sha1);
for (i = 0; i  state-nr_written; i++)
@@ -53,7 +55,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
*state)
 clear_exit:
free(state-written);
memset(state, 0, sizeof(*state));
-
+   free(packname.buf);
/* Make objects we just wrote available to ourselves */
reprepare_packed_git();
 }
-- 
1.7.9.5

 Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling 
 packname, and explain why this is useful.
 Also check if the first argument of pack-write.c:finish_tmp_packfile() can be 
 made const.

Adding 64 to strlen(get_object_directory()) to accomodate sha1_to_hex(sha1) and 
itself.
Using the APIs for strbuf is giving me test failures(12/15) during 
t1050-large.sh 
So, I used the malloc() and free() instead.
Instead of having packname on stack and cause stackoverflow because of MAX_PATH 
~ 4KB, have it on heap.
Can have first parameter to pack-write.c:finish_tmp_packfile() as const because 
packname is not required to be modified.

I apologise for my two earlier patches not being in proper format. I have 
finally got it working properly. Will make sure,
it does not happen again.
--
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] GSoC2014 Microproject rewrite finish_bulk_checkin()

2014-02-27 Thread Faiz Kothari 
Hi,
Thanks for the remarks.
I'll stick to this micro project and follow the guidelines.
Yes, the strbuf API is perfectly OK. I was not getting to work it
properly, so I used malloc() / free() instead. My bad.
I'll resubmit the patch.
Thanks.

On Fri, Feb 28, 2014 at 3:47 AM, Michael Haggerty mhag...@alum.mit.edu wrote:
 On 02/27/2014 08:02 PM, Faiz Kothari wrote:
 Signed-off-by: Faiz Kothari faiz.of...@gmail.com
 ---
  bulk-checkin.c |   12 +++-
  1 file changed, 7 insertions(+), 5 deletions(-)

 diff --git a/bulk-checkin.c b/bulk-checkin.c
 index 118c625..feeff9f 100644
 --- a/bulk-checkin.c
 +++ b/bulk-checkin.c
 @@ -23,7 +23,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;
   int i;

   if (!state-f)
 @@ -42,9 +42,11 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
 *state)
state-offset);
   close(fd);
   }
 -
 - sprintf(packname, %s/pack/pack-, get_object_directory());
 - finish_tmp_packfile(packname, state-pack_tmp_name,
 +
 + packname.len = packname.alloc = 64 + strlen(get_object_directory());
 + packname.buf = (char *)malloc(packname.len * sizeof(char));
 + sprintf(packname.buf, %s/pack/pack-, get_object_directory());
 + finish_tmp_packfile(packname.buf, state-pack_tmp_name,
   state-written, state-nr_written,
   state-pack_idx_opts, sha1);
   for (i = 0; i  state-nr_written; i++)
 @@ -53,7 +55,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
 *state)
  clear_exit:
   free(state-written);
   memset(state, 0, sizeof(*state));
 -
 + free(packname.buf);
   /* Make objects we just wrote available to ourselves */
   reprepare_packed_git();
  }
 -- 1.7.9.5
 Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling 
 packname, and explain why this is useful.
 Also check if the first argument of pack-write.c:finish_tmp_packfile() can 
 be made const.

 Adding 64 to strlen(get_object_directory()) to accomodate sha1_to_hex(sha1) 
 and itself.
 Using the APIs for strbuf is giving me test failures(12/15) during 
 t1050-large.sh
 So, I used the malloc() and free() instead.

 This is not OK.  I promise you, the strbuf API works correctly if it is
 used correctly.  (And if it really *were* broken, you should fix the
 problem or at least diagnose and document it rather than working around it.)

 Instead of having packname on stack and cause stackoverflow because of 
 MAX_PATH ~ 4KB, have it on heap.
 Can have first parameter to pack-write.c:finish_tmp_packfile() as const 
 because packname is not required to be modified.

 I apologise for my two earlier patches not being in proper format. I have 
 finally got it working properly. Will make sure,
 it does not happen again.

 Almost.  This last set of comments should be moved to directly after the
 --- line.

 But: please rather stick to *one* microproject and get it perfect, and
 leave the others to other students.

 Michael

 --
 Michael Haggerty
 mhag...@alum.mit.edu
 http://softwareswirl.blogspot.com/
--
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] Problem in bulk-checkin.c:finish_bulk_checkin() Unable to fix

2014-02-27 Thread Faiz Kothari 
Signed-off-by: Faiz Kothari faiz.of...@gmail.com
---
Compiles without errors.
Fails in test t/t1050-large.sh ,fails 12/15 tests. Dumps memory map and 
backtrace.
Somewhere its not able to free(): invalid pointer.
Please somone pointout where I am doing it wrong.
Help is really appreciated.
Thanks.

 bulk-checkin.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 118c625..c76cd6b 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -23,7 +23,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)
@@ -42,9 +42,8 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
*state)
 state-offset);
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.buf, state-pack_tmp_name,
state-written, state-nr_written,
state-pack_idx_opts, sha1);
for (i = 0; i  state-nr_written; i++)
@@ -53,6 +52,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
*state)
 clear_exit:
free(state-written);
memset(state, 0, sizeof(*state));
+   strbuf_release(packname);
 
/* Make objects we just wrote available to ourselves */
reprepare_packed_git();
-- 
1.7.9.5

--
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 bulk-checkin.c:finish_bulk_checkin() using strbuf

2014-02-27 Thread Faiz Kothari 
Signed-off-by: Faiz Kothari faiz.of...@gmail.com

Notes:
I finally got what's happening, and why the errors were caused.
packname is supposed to contain the complete path to the .pack file.
Packs are stored as /path/to/SHA1.pack which I overlooked earlier.
After inspecting what is happening in pack-write.c:finish_tmp_packfile()
which indirectly modifies packname by appending the SHA1 and .pack to 
packname
This is happening in these code snippets:
char *end_of_name_prefix = strrchr(name_buffer, 0);

and later
sprintf(end_of_name_prefix, %s.pack, sha1_to_hex(sha1));

name_buffer is packname.buf
Using const for the first argument of pack-write.c:finish_tmp_packfile()
doesnot raise any compile time warning or error and not any runtime errors,
since the packname.buf is on heap and has extra space to which more char 
can be written.
If this was not the case,
for e.g. passing a constant string and modifying it.
This will result in a segmentation fault.
---
 bulk-checkin.c |8 +---
 pack-write.c   |2 +-
 pack.h |2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 118c625..bbdf1ec 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -23,7 +23,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)
@@ -42,9 +42,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
*state)
 state-offset);
close(fd);
}
+   strbuf_addf(packname, %s/pack/pack-, get_object_directory());
+   strbuf_grow(packname, 40 + 5);
 
-   sprintf(packname, %s/pack/pack-, get_object_directory());
-   finish_tmp_packfile(packname, state-pack_tmp_name,
+   finish_tmp_packfile(packname.buf, state-pack_tmp_name,
state-written, state-nr_written,
state-pack_idx_opts, sha1);
for (i = 0; i  state-nr_written; i++)
@@ -53,6 +54,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
*state)
 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 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,
 const char *pack_tmp_name,
 struct pack_idx_entry **written_list,
 uint32_t nr_written,
diff --git a/pack.h b/pack.h
index 12d9516..3b9e033 100644
--- a/pack.h
+++ b/pack.h
@@ -91,6 +91,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(const 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[]);
 
 #endif
-- 
1.7.9.5

--
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] GSoC 2014 Microproject 1 rewrite skip_prefix() as loop

2014-02-26 Thread Faiz Kothari 
Hi,
I am Faiz Kothari, I am a GSoC aspirant and want to contribute to git.
I am submitting the patch in reponse to Microproject 1,
rewrite git-compat-util.h:skip_prefix() as a loop.

Signed-off-by: Faiz Kothari faiz.of...@gmail.com
---
 git-compat-util.h | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index cbd86c3..bb2582a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -357,8 +357,11 @@ extern int suffixcmp(const char *str, const char
*suffix);
 
 static inline const char *skip_prefix(const char *str, const char
*prefix)
 {
-   size_t len = strlen(prefix);
-   return strncmp(str, prefix, len) ? NULL : str + len;
+   for (; ; str++, prefix++)
+   if (!*prefix)
+   return str;//code same as strbuf.c:starts_with()
+   else if (*str != *prefix)
+   return NULL;
 }
 
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
-- 
1.9.0.1.ge8df331



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