[PATCH] implemented strbuf_write_or_die()

[PATCH] implemented strbuf_write_or_die()

[Faiz Kothari]

Signed-off-by: Faiz Kothari <faiz.off93@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 db223e8..8f953a2 100644
--- a/cache.h
+++ b/cache.h
@@ -1227,6 +1227,7 @@ extern int copy_fd(int ifd, int ofd);
 extern int copy_file(const char *dst, const char *src, int mode);
 extern int copy_file_with_time(const char *dst, const char *src, int mode);
 extern void write_or_die(int fd, const void *buf, size_t count);
+extern void strbuf_write_or_die(int fd, const struct strbuf *sbuf);
 extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);
 extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);
 extern void fsync_or_die(int fd, const char *);
diff --git a/credential-store.c b/credential-store.c
index f9146e5..5d2f211 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -48,7 +48,7 @@ static void print_entry(struct credential *c)
 static void print_line(struct strbuf *buf)
 {
 	strbuf_addch(buf, '\n');
-	write_or_die(credential_lock.fd, buf->buf, buf->len);
+	strbuf_write_or_die(credential_lock.fd, buf);
 }
 
 static void rewrite_credential_file(const char *fn, struct credential *c,
diff --git a/fetch-pack.c b/fetch-pack.c
index f061f1f..a2bf881 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -216,7 +216,7 @@ static void send_request(struct fetch_pack_args *args,
 		send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);
 		packet_flush(fd);
 	} else
-		write_or_die(fd, buf->buf, buf->len);
+		strbuf_write_or_die(fd, buf);
 }
 
 static void insert_one_alternate_ref(const struct ref *ref, void *unused)
diff --git a/http-backend.c b/http-backend.c
index d2c0a62..0204529 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -157,7 +157,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)
 	hdr_int(content_length, buf->len);
 	hdr_str(content_type, type);
 	end_headers();
-	write_or_die(1, buf->buf, buf->len);
+	strbuf_write_or_die(1, buf);
 }
 
 static void send_local_file(const char *the_type, const char *name)
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);
 
@@ -767,7 +767,8 @@ static int fetch_git(struct discovery *heads,
 
 	err = rpc_service(&rpc, heads);
 	if (rpc.result.len)
-		write_or_die(1, rpc.result.buf, rpc.result.len);
+		strbuf_write_or_die(1, &(rpc.result.buf));
+	
 	strbuf_release(&rpc.result);
 	strbuf_release(&preamble);
 	free(depth_arg);
@@ -889,7 +890,8 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
 
 	err = rpc_service(&rpc, heads);
 	if (rpc.result.len)
-		write_or_die(1, rpc.result.buf, rpc.result.len);
+		strbuf_write_or_die(1, &(rpc.result.buf));
+	
 	strbuf_release(&rpc.result);
 	argv_array_clear(&args);
 	return err;
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"
 
 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)
+{
+	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) {
-- 
1.7.9.5

Re: [PATCH] implemented strbuf_write_or_die()

[He Sun]

2014-03-01 19:21 GMT+08:00 Faiz Kothari <faiz.off93@gmail.com>:
> Signed-off-by: Faiz Kothari <faiz.off93@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 db223e8..8f953a2 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1227,6 +1227,7 @@ extern int copy_fd(int ifd, int ofd);
>  extern int copy_file(const char *dst, const char *src, int mode);
>  extern int copy_file_with_time(const char *dst, const char *src, int mode);
>  extern void write_or_die(int fd, const void *buf, size_t count);
> +extern void strbuf_write_or_die(int fd, const struct strbuf *sbuf);
>  extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);
>  extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);
>  extern void fsync_or_die(int fd, const char *);
> diff --git a/credential-store.c b/credential-store.c
> index f9146e5..5d2f211 100644
> --- a/credential-store.c
> +++ b/credential-store.c
> @@ -48,7 +48,7 @@ static void print_entry(struct credential *c)
>  static void print_line(struct strbuf *buf)
>  {
>         strbuf_addch(buf, '\n');
> -       write_or_die(credential_lock.fd, buf->buf, buf->len);
> +       strbuf_write_or_die(credential_lock.fd, buf);
>  }
>
>  static void rewrite_credential_file(const char *fn, struct credential *c,
> diff --git a/fetch-pack.c b/fetch-pack.c
> index f061f1f..a2bf881 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -216,7 +216,7 @@ static void send_request(struct fetch_pack_args *args,
>                 send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);
>                 packet_flush(fd);
>         } else
> -               write_or_die(fd, buf->buf, buf->len);
> +               strbuf_write_or_die(fd, buf);
>  }
>
>  static void insert_one_alternate_ref(const struct ref *ref, void *unused)
> diff --git a/http-backend.c b/http-backend.c
> index d2c0a62..0204529 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -157,7 +157,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)
>         hdr_int(content_length, buf->len);
>         hdr_str(content_type, type);
>         end_headers();
> -       write_or_die(1, buf->buf, buf->len);
> +       strbuf_write_or_die(1, buf);
>  }
>
>  static void send_local_file(const char *the_type, const char *name)
> 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.

>
> @@ -767,7 +767,8 @@ static int fetch_git(struct discovery *heads,
>
>         err = rpc_service(&rpc, heads);
>         if (rpc.result.len)
> -               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));

> +
>         strbuf_release(&rpc.result);
>         strbuf_release(&preamble);
>         free(depth_arg);
> @@ -889,7 +890,8 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
>
>         err = rpc_service(&rpc, heads);
>         if (rpc.result.len)
> -               write_or_die(1, rpc.result.buf, rpc.result.len);
> +               strbuf_write_or_die(1, &(rpc.result.buf));

Same as above

> +
>         strbuf_release(&rpc.result);
>         argv_array_clear(&args);
>         return err;
> 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"
>
>  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)
> +{
> +       if(write_in_full(fd, sbuf->buf, sbuf->len) < 0){
> +               check_pipe(errno);
> +               die_errno("write error");
> +       }
> +}
> +

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.

>  int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)
>  {
>         if (write_in_full(fd, buf, count) < 0) {
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] implemented strbuf_write_or_die()

[Faiz Kothari]

Signed-off-by: Faiz Kothari <faiz.off93@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);
 				object->flags |= UNINTERESTING;
diff --git a/cache.h b/cache.h
index db223e8..8f953a2 100644
--- a/cache.h
+++ b/cache.h
@@ -1227,6 +1227,7 @@ extern int copy_fd(int ifd, int ofd);
 extern int copy_file(const char *dst, const char *src, int mode);
 extern int copy_file_with_time(const char *dst, const char *src, int mode);
 extern void write_or_die(int fd, const void *buf, size_t count);
+extern void strbuf_write_or_die(int fd, const struct strbuf *sbuf);
 extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);
 extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);
 extern void fsync_or_die(int fd, const char *);
diff --git a/credential-store.c b/credential-store.c
index f9146e5..5d2f211 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -48,7 +48,7 @@ static void print_entry(struct credential *c)
 static void print_line(struct strbuf *buf)
 {
 	strbuf_addch(buf, '\n');
-	write_or_die(credential_lock.fd, buf->buf, buf->len);
+	strbuf_write_or_die(credential_lock.fd, buf);
 }
 
 static void rewrite_credential_file(const char *fn, struct credential *c,
diff --git a/fetch-pack.c b/fetch-pack.c
index f061f1f..a2bf881 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -216,7 +216,7 @@ static void send_request(struct fetch_pack_args *args,
 		send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);
 		packet_flush(fd);
 	} else
-		write_or_die(fd, buf->buf, buf->len);
+		strbuf_write_or_die(fd, buf);
 }
 
 static void insert_one_alternate_ref(const struct ref *ref, void *unused)
diff --git a/http-backend.c b/http-backend.c
index d2c0a62..0204529 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -157,7 +157,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)
 	hdr_int(content_length, buf->len);
 	hdr_str(content_type, type);
 	end_headers();
-	write_or_die(1, buf->buf, buf->len);
+	strbuf_write_or_die(1, buf);
 }
 
 static void send_local_file(const char *the_type, const char *name)
diff --git a/remote-curl.c b/remote-curl.c
index 10cb011..7d45e5e 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);
 
@@ -767,7 +767,8 @@ static int fetch_git(struct discovery *heads,
 
 	err = rpc_service(&rpc, heads);
 	if (rpc.result.len)
-		write_or_die(1, rpc.result.buf, rpc.result.len);
+		strbuf_write_or_die(1, &(rpc.result));
+	
 	strbuf_release(&rpc.result);
 	strbuf_release(&preamble);
 	free(depth_arg);
@@ -889,7 +890,8 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
 
 	err = rpc_service(&rpc, heads);
 	if (rpc.result.len)
-		write_or_die(1, rpc.result.buf, rpc.result.len);
+		strbuf_write_or_die(1, &(rpc.result));
+	
 	strbuf_release(&rpc.result);
 	argv_array_clear(&args);
 	return err;
diff --git a/write_or_die.c b/write_or_die.c
index b50f99a..7ed9821 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "strbuf.h"
 
 static void check_pipe(int err)
 {
@@ -64,6 +65,11 @@ void write_or_die(int fd, const void *buf, size_t count)
 	}
 }
 
+void strbuf_write_or_die(int fd, const struct strbuf *sbuf)
+{
+	write_or_die(fd, sbuf->buf, sbuf->len);
+}
+
 int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)
 {
 	if (write_in_full(fd, buf, count) < 0) {
-- 
1.7.9.5

Re: [PATCH] implemented strbuf_write_or_die()

[Michael Haggerty]

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.off93@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
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

[PATCH v2] implemented strbuf_write_or_die()

[Faiz Kothari]

Signed-off-by: Faiz Kothari <faiz.off93@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') != 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/cache.h b/cache.h
index db223e8..8898bf4 100644
--- a/cache.h
+++ b/cache.h
@@ -1227,6 +1227,7 @@ extern int copy_fd(int ifd, int ofd);
 extern int copy_file(const char *dst, const char *src, int mode);
 extern int copy_file_with_time(const char *dst, const char *src, int mode);
 extern void write_or_die(int fd, const void *buf, size_t count);
+extern void strbuf_write_or_die(const struct strbuf *sbuf, int fd);
 extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);
 extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);
 extern void fsync_or_die(int fd, const char *);
diff --git a/fetch-pack.c b/fetch-pack.c
index f061f1f..af28bd2 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -216,7 +216,7 @@ static void send_request(struct fetch_pack_args *args,
 		send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);
 		packet_flush(fd);
 	} else
-		write_or_die(fd, buf->buf, buf->len);
+		strbuf_write_or_die(buf, fd);
 }
 
 static void insert_one_alternate_ref(const struct ref *ref, void *unused)
diff --git a/http-backend.c b/http-backend.c
index d2c0a62..bed9341 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -157,7 +157,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)
 	hdr_int(content_length, buf->len);
 	hdr_str(content_type, type);
 	end_headers();
-	write_or_die(1, buf->buf, buf->len);
+	strbuf_write_or_die(buf, 1);
 }
 
 static void send_local_file(const char *the_type, const char *name)
diff --git a/remote-curl.c b/remote-curl.c
index 10cb011..677df24 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(preamble, client.in);
 	if (heads)
 		write_or_die(client.in, heads->buf, heads->len);
 
@@ -767,7 +767,7 @@ static int fetch_git(struct discovery *heads,
 
 	err = rpc_service(&rpc, heads);
 	if (rpc.result.len)
-		write_or_die(1, rpc.result.buf, rpc.result.len);
+		strbuf_write_or_die(&rpc.result, 1);
 	strbuf_release(&rpc.result);
 	strbuf_release(&preamble);
 	free(depth_arg);
@@ -889,7 +889,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
 
 	err = rpc_service(&rpc, heads);
 	if (rpc.result.len)
-		write_or_die(1, rpc.result.buf, rpc.result.len);
+		strbuf_write_or_die(&rpc.result, 1);
 	strbuf_release(&rpc.result);
 	argv_array_clear(&args);
 	return err;
diff --git a/write_or_die.c b/write_or_die.c
index b50f99a..373450e 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -64,6 +64,16 @@ void write_or_die(int fd, const void *buf, size_t count)
 	}
 }
 
+void strbuf_write_or_die(const struct strbuf *sbuf, int fd)
+{
+	if(!sbuf)
+		fprintf(stderr, "write error\n");
+	else 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) {
-- 
1.7.9.5

Re: [PATCH v2] implemented strbuf_write_or_die()

[Eric Sunshine]

On Sat, Mar 1, 2014 at 7:18 PM, Faiz Kothari <faiz.off93@gmail.com> wrote:
> Subject: implemented strbuf_write_or_die()

Imperative tone is preferred. The commit message tells what it is
doing, not what it did.

  Subject: introduce strbuf_write_or_die()

> Signed-off-by: Faiz Kothari <faiz.off93@gmail.com>
> ---
> 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.

These three sentences which justify the change should go above the
"---" line so they are recorded in the project history for future
readers to understand why the change was made. As for their actual
value, the first and third sentences are nebulous; the second sentence
may be suitable (although strbuf's buf and len are actually considered
public, so the justification not be supportable).

A couple other comments:

Justification is important because many topics are in-flight at any
given time. Changes like this one which touch an arbitrary number of
files may conflict with other in-flight topics, which places extra
burden on the project maintainer (Junio). The justification needs to
be strong enough to outweigh that added burden.

Introduction of a new function is conceptually distinct from the act
of updating existing callers to invoke it, thus this submission should
probably be decomposed into two or more patches where the first patch
introduces the function, and following patches convert existing
callers.

More below.

> diff --git a/cache.h b/cache.h
> index db223e8..8898bf4 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1227,6 +1227,7 @@ extern int copy_fd(int ifd, int ofd);
>  extern int copy_file(const char *dst, const char *src, int mode);
>  extern int copy_file_with_time(const char *dst, const char *src, int mode);
>  extern void write_or_die(int fd, const void *buf, size_t count);
> +extern void strbuf_write_or_die(const struct strbuf *sbuf, int fd);

You still have the layering violation here mentioned by both Johannes
and Michael. This declaration belongs in strbuf.h, not cache.h

Also, for bonus points, you should document this new function in
Documentation/technical/api-strbuf.txt.

>  extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);
>  extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);
>  extern void fsync_or_die(int fd, const char *);
> diff --git a/write_or_die.c b/write_or_die.c
> index b50f99a..373450e 100644
> --- a/write_or_die.c
> +++ b/write_or_die.c
> @@ -64,6 +64,16 @@ void write_or_die(int fd, const void *buf, size_t count)
>         }
>  }
>
> +void strbuf_write_or_die(const struct strbuf *sbuf, int fd)

Ditto regarding the layering violation. This implementation belongs in strbuf.c.

> +{
> +       if(!sbuf)
> +               fprintf(stderr, "write error\n");

This is a programmer error, rather than a run-time I/O error. As such,
an assertion would be appropriate:

    assert(sbuf);

> +       else if (write_in_full(fd, sbuf->buf, sbuf->len) < 0) {
> +               check_pipe(errno);
> +               die_errno("write error");

He Sun correctly pointed out in his review that you should simply
invoke the lower-level write_or_die() here rather than copying/pasting
its functionality. (You fixed it in an earlier version of the patch
but somehow reverted that fix when sending this version.)

> +       }
> +}

Re: [PATCH v2] implemented strbuf_write_or_die()

[Eric Sunshine]

On Sat, Mar 1, 2014 at 9:47 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Mar 1, 2014 at 7:18 PM, Faiz Kothari <faiz.off93@gmail.com> wrote:
>> 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.
>
> These three sentences which justify the change should go above the
> "---" line so they are recorded in the project history for future
> readers to understand why the change was made. As for their actual
> value, the first and third sentences are nebulous; the second sentence
> may be suitable (although strbuf's buf and len are actually considered
> public, so the justification not be supportable).

...justification *may* not be...

[PATCH v3 1/2] Introduce strbuf_write_or_die()

[Faiz Kothari]

Signed-off-by: Faiz Kothari <faiz.off93@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

[PATCH v3 2/2] use strbuf_write_or_die()

[Faiz Kothari]

Signed-off-by: Faiz Kothari <faiz.off93@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/credential-store.c
index f9146e5..6cb5fcb 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -48,7 +48,7 @@ static void print_entry(struct credential *c)
 static void print_line(struct strbuf *buf)
 {
 	strbuf_addch(buf, '\n');
-	write_or_die(credential_lock.fd, buf->buf, buf->len);
+	strbuf_write_or_die(buf, credential_lock.fd);
 }
 
 static void rewrite_credential_file(const char *fn, struct credential *c,
diff --git a/fetch-pack.c b/fetch-pack.c
index f061f1f..af28bd2 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -216,7 +216,7 @@ static void send_request(struct fetch_pack_args *args,
 		send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);
 		packet_flush(fd);
 	} else
-		write_or_die(fd, buf->buf, buf->len);
+		strbuf_write_or_die(buf, fd);
 }
 
 static void insert_one_alternate_ref(const struct ref *ref, void *unused)
diff --git a/http-backend.c b/http-backend.c
index d2c0a62..bed9341 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -157,7 +157,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)
 	hdr_int(content_length, buf->len);
 	hdr_str(content_type, type);
 	end_headers();
-	write_or_die(1, buf->buf, buf->len);
+	strbuf_write_or_die(buf, 1);
 }
 
 static void send_local_file(const char *the_type, const char *name)
diff --git a/remote-curl.c b/remote-curl.c
index 10cb011..677df24 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(preamble, client.in);
 	if (heads)
 		write_or_die(client.in, heads->buf, heads->len);
 
@@ -767,7 +767,7 @@ static int fetch_git(struct discovery *heads,
 
 	err = rpc_service(&rpc, heads);
 	if (rpc.result.len)
-		write_or_die(1, rpc.result.buf, rpc.result.len);
+		strbuf_write_or_die(&rpc.result, 1);
 	strbuf_release(&rpc.result);
 	strbuf_release(&preamble);
 	free(depth_arg);
@@ -889,7 +889,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
 
 	err = rpc_service(&rpc, heads);
 	if (rpc.result.len)
-		write_or_die(1, rpc.result.buf, rpc.result.len);
+		strbuf_write_or_die(&rpc.result, 1);
 	strbuf_release(&rpc.result);
 	argv_array_clear(&args);
 	return err;
-- 
1.7.9.5

Re: [PATCH v3 2/2] use strbuf_write_or_die()

[Eric Sunshine]

On Sun, Mar 2, 2014 at 2:34 AM, Faiz Kothari <faiz.off93@gmail.com> wrote:
> Signed-off-by: Faiz Kothari <faiz.off93@gmail.com>

Sign off below commit message.

> Used strbuf.c:strbuf_write_or_die() instead of
> write_or_die.c:write_or_die() at relevant places.

Imperative: "Use strbuf..."

Otherwise, the patch looks okay.

> ---
>  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/credential-store.c
> index f9146e5..6cb5fcb 100644
> --- a/credential-store.c
> +++ b/credential-store.c
> @@ -48,7 +48,7 @@ static void print_entry(struct credential *c)
>  static void print_line(struct strbuf *buf)
>  {
>         strbuf_addch(buf, '\n');
> -       write_or_die(credential_lock.fd, buf->buf, buf->len);
> +       strbuf_write_or_die(buf, credential_lock.fd);
>  }
>
>  static void rewrite_credential_file(const char *fn, struct credential *c,
> diff --git a/fetch-pack.c b/fetch-pack.c
> index f061f1f..af28bd2 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -216,7 +216,7 @@ static void send_request(struct fetch_pack_args *args,
>                 send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);
>                 packet_flush(fd);
>         } else
> -               write_or_die(fd, buf->buf, buf->len);
> +               strbuf_write_or_die(buf, fd);
>  }
>
>  static void insert_one_alternate_ref(const struct ref *ref, void *unused)
> diff --git a/http-backend.c b/http-backend.c
> index d2c0a62..bed9341 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -157,7 +157,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)
>         hdr_int(content_length, buf->len);
>         hdr_str(content_type, type);
>         end_headers();
> -       write_or_die(1, buf->buf, buf->len);
> +       strbuf_write_or_die(buf, 1);
>  }
>
>  static void send_local_file(const char *the_type, const char *name)
> diff --git a/remote-curl.c b/remote-curl.c
> index 10cb011..677df24 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(preamble, client.in);
>         if (heads)
>                 write_or_die(client.in, heads->buf, heads->len);
>
> @@ -767,7 +767,7 @@ static int fetch_git(struct discovery *heads,
>
>         err = rpc_service(&rpc, heads);
>         if (rpc.result.len)
> -               write_or_die(1, rpc.result.buf, rpc.result.len);
> +               strbuf_write_or_die(&rpc.result, 1);
>         strbuf_release(&rpc.result);
>         strbuf_release(&preamble);
>         free(depth_arg);
> @@ -889,7 +889,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
>
>         err = rpc_service(&rpc, heads);
>         if (rpc.result.len)
> -               write_or_die(1, rpc.result.buf, rpc.result.len);
> +               strbuf_write_or_die(&rpc.result, 1);
>         strbuf_release(&rpc.result);
>         argv_array_clear(&args);
>         return err;
> --
> 1.7.9.5
>

Re: [PATCH v3 1/2] Introduce strbuf_write_or_die()

[Eric Sunshine]

On Sun, Mar 2, 2014 at 2:34 AM, Faiz Kothari <faiz.off93@gmail.com> wrote:
> Signed-off-by: Faiz Kothari <faiz.off93@gmail.com>

Place your sign off below the commit message.

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

You want to explain what this patch is doing in imperative tone. Use
"Introduce" rather than "Introduced". The first sentence correctly
states what the patch is doing, however, the second sentence explains
what the next patch is doing, so it doesn't belong here. So, your
commit message for this patch might become:

    Subject: strbuf: introduce strbuf_write_or_die()

    Add strbuf convenience wrapper around lower-level 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.

Good explanation of what changed since the last attempt.

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

It's a good idea to add documentation when you add the function
itself, otherwise reviewers will have to wait yet another round to
review that addition. In this case, the documentation will likely be
one line, so it shouldn't be a particular burden to write it.

> 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);
> +}

Nice. Much better than previous versions of the patch.

>  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

Re: [PATCH] implemented strbuf_write_or_die()

[Eric Sunshine]

On Sat, Mar 1, 2014 at 7:51 AM, He Sun <sunheehnus@gmail.com> wrote:
> 2014-03-01 19:21 GMT+08:00 Faiz Kothari <faiz.off93@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.

Re: [PATCH] implemented strbuf_write_or_die()

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sat, Mar 1, 2014 at 7:51 AM, He Sun <sunheehnus@gmail.com> wrote:
>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari <faiz.off93@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.

Fwd: [PATCH] implemented strbuf_write_or_die()

[Faiz Kothari]

On Tue, Mar 4, 2014 at 12:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Sat, Mar 1, 2014 at 7:51 AM, He Sun <sunheehnus@gmail.com> wrote:
>>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari <faiz.off93@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

Re: [PATCH] implemented strbuf_write_or_die()

[Eric Sunshine]

On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Sat, Mar 1, 2014 at 7:51 AM, He Sun <sunheehnus@gmail.com> wrote:
>>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari <faiz.off93@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

Re: [PATCH] implemented strbuf_write_or_die()

[David Kastrup]

Eric Sunshine <sunshine@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

Re: [PATCH] implemented strbuf_write_or_die()

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>>> On Sat, Mar 1, 2014 at 7:51 AM, He Sun <sunheehnus@gmail.com> wrote:
>>>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari <faiz.off93@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 ;-)

Re: [PATCH] implemented strbuf_write_or_die()

[Eric Sunshine]

On Mon, Mar 3, 2014 at 3:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>> On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Eric Sunshine <sunshine@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.

Re: [PATCH] implemented strbuf_write_or_die()

[Michael Haggerty]

On 03/03/2014 07:31 PM, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>> On Sat, Mar 1, 2014 at 7:51 AM, He Sun <sunheehnus@gmail.com> wrote:
>>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari <faiz.off93@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
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH] implemented strbuf_write_or_die()

[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

Re: [PATCH] implemented strbuf_write_or_die()

[Junio C Hamano]

Michael Haggerty <mhagger@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 ;-)

Re: [PATCH] implemented strbuf_write_or_die()

[Johannes Sixt]

Am 01.03.2014 12:21, schrieb Faiz Kothari:
> Signed-off-by: Faiz Kothari <faiz.off93@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

Re: [PATCH] implemented strbuf_write_or_die()

[Junio C Hamano]

Faiz Kothari <faiz.off93@gmail.com> writes:

> Signed-off-by: Faiz Kothari <faiz.off93@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?