* [PATCH] Rename read_pipe() with read_fd() and make its buffer nul-terminated.
@ 2007-07-18 7:08 Carlos Rica
2007-07-18 7:49 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Carlos Rica @ 2007-07-18 7:08 UTC (permalink / raw)
To: git, Junio C Hamano, Johannes Schindelin, Kristian Høgsberg
The new name is closer to the purpose of the function.
The other change just makes things easier for callers needing a
NUL-terminated buffer.
Since the function returns only the memory written with data,
almost always allocating more space than needed because final
size is unknown, an extra NUL terminating the buffer is harmless.
It is not included in the returned size, so the function
remains working as before.
Also, now the function allows the buffer passed to be NULL at first,
and alloc_nr is now used for growing the buffer, instead size=*2.
Signed-off-by: Carlos Rica <jasampler@gmail.com>
---
builtin-stripspace.c | 4 +++-
cache.h | 2 +-
mktag.c | 2 +-
sha1_file.c | 17 ++++++++++++++---
4 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/builtin-stripspace.c b/builtin-stripspace.c
index 0c970aa..5571687 100644
--- a/builtin-stripspace.c
+++ b/builtin-stripspace.c
@@ -79,8 +79,10 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
size = 1024;
buffer = xmalloc(size);
- if (read_pipe(0, &buffer, &size))
+ if (read_fd(0, &buffer, &size)) {
+ free(buffer);
die("could not read the input");
+ }
size = stripspace(buffer, size, 0);
write_or_die(1, buffer, size);
diff --git a/cache.h b/cache.h
index 328c1ad..ec9b43d 100644
--- a/cache.h
+++ b/cache.h
@@ -265,7 +265,7 @@ extern int ie_match_stat(struct index_state *, struct cache_entry *, struct stat
extern int ie_modified(struct index_state *, struct cache_entry *, struct stat *, int);
extern int ce_path_match(const struct cache_entry *ce, const char **pathspec);
extern int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object, enum object_type type, const char *path);
-extern int read_pipe(int fd, char** return_buf, unsigned long* return_size);
+extern int read_fd(int fd, char** return_buf, unsigned long* return_size);
extern int index_pipe(unsigned char *sha1, int fd, const char *type, int write_object);
extern int index_path(unsigned char *sha1, const char *path, struct stat *st, int write_object);
extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);
diff --git a/mktag.c b/mktag.c
index b82e377..38acd5a 100644
--- a/mktag.c
+++ b/mktag.c
@@ -120,7 +120,7 @@ int main(int argc, char **argv)
setup_git_directory();
- if (read_pipe(0, &buffer, &size)) {
+ if (read_fd(0, &buffer, &size)) {
free(buffer);
die("could not read from stdin");
}
diff --git a/sha1_file.c b/sha1_file.c
index 1efd9ae..563ec07 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2304,27 +2304,38 @@ int has_sha1_file(const unsigned char *sha1)
*
* returns 0 if anything went fine and -1 otherwise
*
+ * The buffer is always NUL-terminated, not including it in returned size.
+ *
* NOTE: both buf and size may change, but even when -1 is returned
* you still have to free() it yourself.
*/
-int read_pipe(int fd, char** return_buf, unsigned long* return_size)
+int read_fd(int fd, char** return_buf, unsigned long* return_size)
{
char* buf = *return_buf;
unsigned long size = *return_size;
ssize_t iret;
unsigned long off = 0;
+ if (!buf || size <= 1) {
+ size = alloc_nr(size);
+ buf = xrealloc(buf, size);
+ }
+
do {
iret = xread(fd, buf + off, size - off);
if (iret > 0) {
off += iret;
if (off == size) {
- size *= 2;
+ size = alloc_nr(size);
buf = xrealloc(buf, size);
}
}
} while (iret > 0);
+ if (off == size)
+ buf = xrealloc(buf, size + 1);
+ buf[off] = '\0';
+
*return_buf = buf;
*return_size = off;
@@ -2339,7 +2350,7 @@ int index_pipe(unsigned char *sha1, int fd, const char *type, int write_object)
char *buf = xmalloc(size);
int ret;
- if (read_pipe(fd, &buf, &size)) {
+ if (read_fd(fd, &buf, &size)) {
free(buf);
return -1;
}
--
1.5.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Rename read_pipe() with read_fd() and make its buffer nul-terminated.
2007-07-18 7:08 Carlos Rica
@ 2007-07-18 7:49 ` Junio C Hamano
2007-07-18 15:40 ` Carlos Rica
0 siblings, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2007-07-18 7:49 UTC (permalink / raw)
To: Carlos Rica; +Cc: git, Johannes Schindelin, Kristian Høgsberg
Carlos Rica <jasampler@gmail.com> writes:
> The new name is closer to the purpose of the function.
>
> The other change just makes things easier for callers needing a
> NUL-terminated buffer.
>
> Since the function returns only the memory written with data,
> almost always allocating more space than needed because final
> size is unknown, an extra NUL terminating the buffer is harmless.
> It is not included in the returned size, so the function
> remains working as before.
>
> Also, now the function allows the buffer passed to be NULL at first,
> and alloc_nr is now used for growing the buffer, instead size=*2.
Some people may say function name change should be a separate
patch by itself, but I'd let it pass for now...
> diff --git a/sha1_file.c b/sha1_file.c
> index 1efd9ae..563ec07 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2304,27 +2304,38 @@ int has_sha1_file(const unsigned char *sha1)
> *
> * returns 0 if anything went fine and -1 otherwise
> *
> + * The buffer is always NUL-terminated, not including it in returned size.
> + *
> * NOTE: both buf and size may change, but even when -1 is returned
> * you still have to free() it yourself.
> */
> -int read_pipe(int fd, char** return_buf, unsigned long* return_size)
> +int read_fd(int fd, char** return_buf, unsigned long* return_size)
> {
> char* buf = *return_buf;
Our code seems to prefer pointer declaration to be spelled as
"type **var", not "type** var".
> unsigned long size = *return_size;
> ssize_t iret;
> unsigned long off = 0;
>
> + if (!buf || size <= 1) {
> + size = alloc_nr(size);
> + buf = xrealloc(buf, size);
> + }
> +
Hmph. The reason this is not "!size" is because you would want
more than one. As your plan is to use this mostly for the -F
option of "tag/commit", I suspect using a bit larger default,
such as 80 (just a line), or probably 1k (most log messages
would fit in such a buffer), would be more practical.
> do {
> iret = xread(fd, buf + off, size - off);
> if (iret > 0) {
> off += iret;
> if (off == size) {
> - size *= 2;
> + size = alloc_nr(size);
> buf = xrealloc(buf, size);
> }
> }
> } while (iret > 0);
>
> + if (off == size)
> + buf = xrealloc(buf, size + 1);
> + buf[off] = '\0';
> +
I wonder if doing xread(... (size-1) - off) in the loop would
ensure (off <= size-1) here. You also would need to update the
realloc condition inside loop if you do so.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Rename read_pipe() with read_fd() and make its buffer nul-terminated.
2007-07-18 7:49 ` Junio C Hamano
@ 2007-07-18 15:40 ` Carlos Rica
0 siblings, 0 replies; 4+ messages in thread
From: Carlos Rica @ 2007-07-18 15:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin, Kristian Høgsberg
2007/7/18, Junio C Hamano <gitster@pobox.com>:
>
> > unsigned long size = *return_size;
> > ssize_t iret;
> > unsigned long off = 0;
> >
> > + if (!buf || size <= 1) {
> > + size = alloc_nr(size);
> > + buf = xrealloc(buf, size);
> > + }
> > +
>
> Hmph. The reason this is not "!size" is because you would want
> more than one. As your plan is to use this mostly for the -F
> option of "tag/commit", I suspect using a bit larger default,
> such as 80 (just a line), or probably 1k (most log messages
> would fit in such a buffer), would be more practical.
The true reason is because I before tried using (size-1) instead of (size)
in xread(..., size - off), and then I forgot to remove that condition. Sorry.
>
> > do {
> > iret = xread(fd, buf + off, size - off);
> > if (iret > 0) {
> > off += iret;
> > if (off == size) {
> > - size *= 2;
> > + size = alloc_nr(size);
> > buf = xrealloc(buf, size);
> > }
> > }
> > } while (iret > 0);
> >
> > + if (off == size)
> > + buf = xrealloc(buf, size + 1);
> > + buf[off] = '\0';
> > +
>
> I wonder if doing xread(... (size-1) - off) in the loop would
> ensure (off <= size-1) here. You also would need to update the
> realloc condition inside loop if you do so.
I have chosen that solution to leave the code easy to read, but the
(size-1) version will avoid that additional (but non-frequent) realloc.
I will resend the fixed patch along with the builtin-tag.c one.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] Rename read_pipe() with read_fd() and make its buffer nul-terminated.
@ 2007-07-18 18:31 Carlos Rica
0 siblings, 0 replies; 4+ messages in thread
From: Carlos Rica @ 2007-07-18 18:31 UTC (permalink / raw)
To: git, Junio C Hamano, Johannes Schindelin, Kristian Høgsberg
The new name is closer to the purpose of the function.
A NUL-terminated buffer makes things easier when callers need that.
Since the function returns only the memory written with data,
almost always allocating more space than needed because final
size is unknown, an extra NUL terminating the buffer is harmless.
It is not included in the returned size, so the function
remains working as before.
Also, now the function allows the buffer passed to be NULL at first,
and alloc_nr is now used for growing the buffer, instead size=*2.
Signed-off-by: Carlos Rica <jasampler@gmail.com>
---
builtin-stripspace.c | 4 +++-
cache.h | 2 +-
mktag.c | 2 +-
sha1_file.c | 21 +++++++++++++++------
4 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/builtin-stripspace.c b/builtin-stripspace.c
index 0c970aa..5571687 100644
--- a/builtin-stripspace.c
+++ b/builtin-stripspace.c
@@ -79,8 +79,10 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
size = 1024;
buffer = xmalloc(size);
- if (read_pipe(0, &buffer, &size))
+ if (read_fd(0, &buffer, &size)) {
+ free(buffer);
die("could not read the input");
+ }
size = stripspace(buffer, size, 0);
write_or_die(1, buffer, size);
diff --git a/cache.h b/cache.h
index 328c1ad..53801b8 100644
--- a/cache.h
+++ b/cache.h
@@ -265,7 +265,7 @@ extern int ie_match_stat(struct index_state *, struct cache_entry *, struct stat
extern int ie_modified(struct index_state *, struct cache_entry *, struct stat *, int);
extern int ce_path_match(const struct cache_entry *ce, const char **pathspec);
extern int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object, enum object_type type, const char *path);
-extern int read_pipe(int fd, char** return_buf, unsigned long* return_size);
+extern int read_fd(int fd, char **return_buf, unsigned long *return_size);
extern int index_pipe(unsigned char *sha1, int fd, const char *type, int write_object);
extern int index_path(unsigned char *sha1, const char *path, struct stat *st, int write_object);
extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);
diff --git a/mktag.c b/mktag.c
index b82e377..38acd5a 100644
--- a/mktag.c
+++ b/mktag.c
@@ -120,7 +120,7 @@ int main(int argc, char **argv)
setup_git_directory();
- if (read_pipe(0, &buffer, &size)) {
+ if (read_fd(0, &buffer, &size)) {
free(buffer);
die("could not read from stdin");
}
diff --git a/sha1_file.c b/sha1_file.c
index 1efd9ae..aca741b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2304,27 +2304,36 @@ int has_sha1_file(const unsigned char *sha1)
*
* returns 0 if anything went fine and -1 otherwise
*
+ * The buffer is always NUL-terminated, not including it in returned size.
+ *
* NOTE: both buf and size may change, but even when -1 is returned
* you still have to free() it yourself.
*/
-int read_pipe(int fd, char** return_buf, unsigned long* return_size)
+int read_fd(int fd, char **return_buf, unsigned long *return_size)
{
- char* buf = *return_buf;
+ char *buf = *return_buf;
unsigned long size = *return_size;
ssize_t iret;
unsigned long off = 0;
+ if (!buf || size <= 1) {
+ size = 1024;
+ buf = xrealloc(buf, size);
+ }
+
do {
- iret = xread(fd, buf + off, size - off);
+ iret = xread(fd, buf + off, (size - 1) - off);
if (iret > 0) {
off += iret;
- if (off == size) {
- size *= 2;
+ if (off == size - 1) {
+ size = alloc_nr(size);
buf = xrealloc(buf, size);
}
}
} while (iret > 0);
+ buf[off] = '\0';
+
*return_buf = buf;
*return_size = off;
@@ -2339,7 +2348,7 @@ int index_pipe(unsigned char *sha1, int fd, const char *type, int write_object)
char *buf = xmalloc(size);
int ret;
- if (read_pipe(fd, &buf, &size)) {
+ if (read_fd(fd, &buf, &size)) {
free(buf);
return -1;
}
--
1.5.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2007-07-18 18:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-07-18 18:31 [PATCH] Rename read_pipe() with read_fd() and make its buffer nul-terminated Carlos Rica
-- strict thread matches above, loose matches on Subject: below --
2007-07-18 7:08 Carlos Rica
2007-07-18 7:49 ` Junio C Hamano
2007-07-18 15:40 ` Carlos Rica
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).