[PATCH] strbuf: move strbuf_readline_fd() from bundle.c to strbuf.{c,h}

[PATCH] strbuf: move strbuf_readline_fd() from bundle.c to strbuf.{c,h}

[徐迪]

strbuf_readline_fd() existed in bundle.c since e9ee84cf, but this
function can be used elsewhere, and since it's relevant to strbuf, it
should be in strbuf.{c,h}.

Signed-off-by: Xu Di <xudifsd@gmail.com>
---
 bundle.c |   18 +-----------------
 strbuf.c |   16 ++++++++++++++++
 strbuf.h |    1 +
 3 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/bundle.c b/bundle.c
index b8acf3c..9344a91 100644
--- a/bundle.c
+++ b/bundle.c
@@ -7,6 +7,7 @@
 #include "list-objects.h"
 #include "run-command.h"
 #include "refs.h"
+#include "strbuf.h"

 static const char bundle_signature[] = "# v2 git bundle\n";

@@ -23,23 +24,6 @@ static void add_to_ref_list(const unsigned char
*sha1, const char *name,
 	list->nr++;
 }

-/* Eventually this should go to strbuf.[ch] */
-static int strbuf_readline_fd(struct strbuf *sb, int fd)
-{
-	strbuf_reset(sb);
-
-	while (1) {
-		char ch;
-		ssize_t len = xread(fd, &ch, 1);
-		if (len <= 0)
-			return len;
-		strbuf_addch(sb, ch);
-		if (ch == '\n')
-			break;
-	}
-	return 0;
-}
-
 static int parse_bundle_header(int fd, struct bundle_header *header,
 			       const char *report_path)
 {
diff --git a/strbuf.c b/strbuf.c
index ff0b96b..7532a13 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -282,6 +282,22 @@ void strbuf_addbuf_percentquote(struct strbuf
*dst, const struct strbuf *src)
 	}
 }

+int strbuf_readline_fd(struct strbuf *sb, int fd)
+{
+	strbuf_reset(sb);
+
+	while (1) {
+		char ch;
+		ssize_t len = xread(fd, &ch, 1);
+		if (len <= 0)
+			return len;
+		strbuf_addch(sb, ch);
+		if (ch == '\n')
+			break;
+	}
+	return 0;
+}
+
 size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
 {
 	size_t res;
diff --git a/strbuf.h b/strbuf.h
index fbf059f..ecebd11 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -109,6 +109,7 @@ static inline void strbuf_complete_line(struct strbuf *sb)
 }

 extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
+extern int strbuf_readline_fd(struct strbuf *sb, int fd);
 /* 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);
-- 
1.7.8.1.749.gb6b3b

Re: [PATCH] strbuf: move strbuf_readline_fd() from bundle.c to strbuf.{c,h}

[徐迪]

Shouldn't I supposed to receive any information about this patch? Has
it accepted or not? If not what wrong with this patch?

在 2012年2月11日 下午5:50，徐迪 <xudifsd@gmail.com> 写道：
> strbuf_readline_fd() existed in bundle.c since e9ee84cf, but this
> function can be used elsewhere, and since it's relevant to strbuf, it
> should be in strbuf.{c,h}.
>
> Signed-off-by: Xu Di <xudifsd@gmail.com>
> ---
>  bundle.c |   18 +-----------------
>  strbuf.c |   16 ++++++++++++++++
>  strbuf.h |    1 +
>  3 files changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/bundle.c b/bundle.c
> index b8acf3c..9344a91 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -7,6 +7,7 @@
>  #include "list-objects.h"
>  #include "run-command.h"
>  #include "refs.h"
> +#include "strbuf.h"
>
>  static const char bundle_signature[] = "# v2 git bundle\n";
>
> @@ -23,23 +24,6 @@ static void add_to_ref_list(const unsigned char
> *sha1, const char *name,
>        list->nr++;
>  }
>
> -/* Eventually this should go to strbuf.[ch] */
> -static int strbuf_readline_fd(struct strbuf *sb, int fd)
> -{
> -       strbuf_reset(sb);
> -
> -       while (1) {
> -               char ch;
> -               ssize_t len = xread(fd, &ch, 1);
> -               if (len <= 0)
> -                       return len;
> -               strbuf_addch(sb, ch);
> -               if (ch == '\n')
> -                       break;
> -       }
> -       return 0;
> -}
> -
>  static int parse_bundle_header(int fd, struct bundle_header *header,
>                               const char *report_path)
>  {
> diff --git a/strbuf.c b/strbuf.c
> index ff0b96b..7532a13 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -282,6 +282,22 @@ void strbuf_addbuf_percentquote(struct strbuf
> *dst, const struct strbuf *src)
>        }
>  }
>
> +int strbuf_readline_fd(struct strbuf *sb, int fd)
> +{
> +       strbuf_reset(sb);
> +
> +       while (1) {
> +               char ch;
> +               ssize_t len = xread(fd, &ch, 1);
> +               if (len <= 0)
> +                       return len;
> +               strbuf_addch(sb, ch);
> +               if (ch == '\n')
> +                       break;
> +       }
> +       return 0;
> +}
> +
>  size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
>  {
>        size_t res;
> diff --git a/strbuf.h b/strbuf.h
> index fbf059f..ecebd11 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -109,6 +109,7 @@ static inline void strbuf_complete_line(struct strbuf *sb)
>  }
>
>  extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
> +extern int strbuf_readline_fd(struct strbuf *sb, int fd);
>  /* 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);
> --
> 1.7.8.1.749.gb6b3b

Re: [PATCH] strbuf: move strbuf_readline_fd() from bundle.c to strbuf.{c,h}

[Junio C Hamano]

徐迪 <xudifsd@gmail.com> writes:

> Shouldn't I supposed to receive any information about this patch?

Not necessarily X-<.

Things can get lost in the noise, and sending a reminder message like you
just did, after waiting for a few to several days, is considered a good
practice.

> Has it accepted or not?

Anything and everything being considered for inclusion is reachable from
the tip of the 'next' branch; other patches that might turn out to be of
value, or ones that at least deserve more sets of eyeballs, are only in
the 'pu' branch.

> If not what wrong with this patch?

That is not a valid question to ask.

We do not apply a patch merely because it is _not wrong_.  It must be
clear why it is a good thing for the project to apply the patch.

Your patch in question is barely a day old and I didn't have a chance to
look at it, and I do not think other volunteer reviewers did, either, so
I don't know if it is wrong or it has merit yet.

Re: [PATCH] strbuf: move strbuf_readline_fd() from bundle.c to strbuf.{c,h}

[徐迪]

>> Shouldn't I supposed to receive any information about this patch?
>
> Not necessarily X-<.
>
> Things can get lost in the noise, and sending a reminder message like you
> just did, after waiting for a few to several days, is considered a good
> practice.
>

Ok, excuse me, but this is my first time to make a patch. I'm
desperately want to know its fate :)

>> Has it accepted or not?
>
> Anything and everything being considered for inclusion is reachable from
> the tip of the 'next' branch; other patches that might turn out to be of
> value, or ones that at least deserve more sets of eyeballs, are only in
> the 'pu' branch.
>

Does this mean that we can check 'next' branch to see if any of our
patches being considered for inclusion?

Re: [PATCH] strbuf: move strbuf_readline_fd() from bundle.c to strbuf.{c,h}

[Junio C Hamano]

徐迪 <xudifsd@gmail.com> writes:

> -/* Eventually this should go to strbuf.[ch] */
> -static int strbuf_readline_fd(struct strbuf *sb, int fd)
> -{

Unlike other line-oriented input functions in strbuf.c, this hardcodes the
LF as the terminator, hence is not capable of reading -z output.  As an
internal function to the bundle reader implementation, that is perfectly
fine, because we know lines in bundle ends with '\n'.

But if we were to promote this to a generic API, that aspect of this
function needs to be reconsidered, and if we decide to allow non-LF line
terminator for consistency with other functions, the current callers in
this function have to pass an extra parameter "term" to it.

We simply do not have enough datapoint to see if it is worth it to promote
this to strbuf.c with such a change, especially when there is no other
caller to it.  That is why we said "Eventually" here when we introduced
this function at e9ee84c (bundle: allowing to read from an unseekable fd,
2011-10-13).

I do not think the situation changed yet in any way since then.