Re: [PATCH] implemented strbuf_write_or_die()
On 03/03/2014 07:31 PM, Junio C Hamano 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. 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 -- 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
Re: [PATCH] implemented strbuf_write_or_die()
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
Re: [PATCH] implemented strbuf_write_or_die()
Michael Haggerty mhag...@alum.mit.edu writes: On 03/03/2014 07:31 PM, Junio C Hamano wrote: 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. ... ... 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. Yeah, I understand that. I suspect that the conclusion would have been very different if we were a C++ project; most likely it would be an excellent idea to add an often-used write_or_die() method to the strbuf class. But we are writing C. Faiz, this is the way things go on the Git mailing list. It would be boring if everybody agreed all the time :-) Surely, and thanks ;-) -- 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()
Faiz Kothari faiz.of...@gmail.com writes: 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(-) It does not reduce the code, it does not make the resulting code read any easier. What is the benefit of this change? -- 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()
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. -- 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()
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] implemented strbuf_write_or_die()
On Mon, Mar 3, 2014 at 1:31 PM, 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. As a potential GSoC student and newcomer to the project, Faiz would not have known that this would be considered unwanted churn when he chose the task from the GSoC microproject page [1]. Perhaps it would be a good idea to retire this item from the list? On the other hand, it did expose Faiz to the iterative code review process on this project and gave him a taste of what would be expected of him as a GSoC student, so the microproject achieved that important goal, and thus wasn't an utter failure. [1]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md -- 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()
Eric Sunshine sunsh...@sunshineco.com writes: As a potential GSoC student and newcomer to the project, Faiz would not have known that this would be considered unwanted churn when he chose the task from the GSoC microproject page [1]. Perhaps it would be a good idea to retire this item from the list? On the other hand, it did expose Faiz to the iterative code review process on this project and gave him a taste of what would be expected of him as a GSoC student, so the microproject achieved that important goal, and thus wasn't an utter failure. And the microproject has the fabulous property that we can use it over and over again to have a newcomer try committing patches: the previously reported problem that we were running out of microprojects will not occur when every patch is eventually going to be rejected. Joking aside, this is a motivational disaster. It should be retired. -- David Kastrup -- 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()
Eric Sunshine sunsh...@sunshineco.com writes: On Mon, Mar 3, 2014 at 1:31 PM, 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. As a potential GSoC student and newcomer to the project, Faiz would not have known that this would be considered unwanted churn when he chose the task from the GSoC microproject page [1]. Perhaps it would be a good idea to retire this item from the list? I don't think I saw this on the microproject suggestion page when I last looked at it, and assumed that this was on the student's own initiative. On the other hand, it did expose Faiz to the iterative code review process on this project and gave him a taste of what would be expected of him as a GSoC student, so the microproject achieved that important goal, and thus wasn't an utter failure. [1]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md Surely. I would have to say that this is not a good sample exercise to suggest to new students and I'd encourage dropping it from the list. You could argue that it is an effective way to cull people with bad design taste to mix suggestions to make the codebase worse and see who picks them, but I do not think it is very fair ;-) -- 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()
On Mon, Mar 3, 2014 at 3:35 PM, Junio C Hamano gits...@pobox.com wrote: Eric Sunshine sunsh...@sunshineco.com writes: On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano gits...@pobox.com wrote: Eric Sunshine sunsh...@sunshineco.com writes: 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. As a potential GSoC student and newcomer to the project, Faiz would not have known that this would be considered unwanted churn when he chose the task from the GSoC microproject page [1]. Perhaps it would be a good idea to retire this item from the list? I don't think I saw this on the microproject suggestion page when I last looked at it, and assumed that this was on the student's own initiative. I also had not seen it earlier on the microprojects page and had the same reaction until I re-checked the page and found that it had been added [1]. The microprojects page already instructs students to indicate that a submission is for GSoC [2] (and many have followed the advice), but perhaps we can avoid this sort of misunderstanding in the future by making it more explicit: for instance, tell them to add [GSoC] to the Subject:. [1]: https://github.com/git/git.github.io/commit/f314120a2b5e831459673c612a3630ad953d9954 [2]: https://github.com/git/git.github.io/blame/master/SoC-2014-Microprojects.md#L83 On the other hand, it did expose Faiz to the iterative code review process on this project and gave him a taste of what would be expected of him as a GSoC student, so the microproject achieved that important goal, and thus wasn't an utter failure. [1]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md Surely. I would have to say that this is not a good sample exercise to suggest to new students and I'd encourage dropping it from the list. You could argue that it is an effective way to cull people with bad design taste to mix suggestions to make the codebase worse and see who picks them, but I do not think it is very fair ;-) Agreed. The item should be dropped from the list. -- 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()
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
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)) {
[PATCH] implemented strbuf_write_or_die()
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);
Re: [PATCH] implemented strbuf_write_or_die()
Am 01.03.2014 12:21, schrieb 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.
You haven't given a justifiction of the change (why is this change good?)
diff --git a/write_or_die.c b/write_or_die.c
index b50f99a..5fb309b 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -1,4 +1,5 @@
#include cache.h
+#include strbuf.h
I think you have the layering backwards here: strbuf_write_or_die should
be part of the (higher-level) strbuf API, and not an extension of the
low-level write_or_die function.
static void check_pipe(int err)
{
@@ -64,6 +65,14 @@ void write_or_die(int fd, const void *buf, size_t count)
}
}
+void strbuf_write_or_die(int fd, const struct strbuf *sbuf)
And when you make the function a strbuf API, the prototype should be
void strbuf_write_or_die(const struct strbuf *sbuf, int fd)
as in hey, strbuf object, write your content to this file descriptor!
+{
+ if(write_in_full(fd, sbuf-buf, sbuf-len) 0){
+ check_pipe(errno);
+ die_errno(write error);
+ }
+}
+
int write_or_whine_pipe(int fd, const void *buf, size_t count, const char
*msg)
{
if (write_in_full(fd, buf, count) 0) {
-- Hannes
--
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()
Please leave a little more time for people to give feedback between versions of a patch series (unless the first version was so broken that it would be pointless for any other reviewer to waste time on it. And please label the versions of a single patch series [PATCH] then [PATCH v2], [PATCH v3], etc. I agree with Johannes's advice that the function is in the wrong place and has the wrong parameter order. On 03/01/2014 02:29 PM, Faiz Kothari wrote: 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. I find it alarming that either the compiler didn't emit warnings for the old version or that you ignored the compiler warnings. Git should compile without warnings even with with quite strict compiler settings; I use gcc with the following options -Wall -Werror \ -Wdeclaration-after-statement \ -Wno-format-zero-length \ -Wno-format-security Maybe you weren't including the header file that declares strbuf_write_or_die() in the file containing this invocation? Also, the parentheses in (rpc.result) are unnecessary. And I think that some of the blank lines that you added contained invisible whitespace. Please check your whitespace! You can run git diff --check to detect some obvious whitespace problems. 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
Re: [PATCH] implemented strbuf_write_or_die()
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. -- 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
