threads / patch / 29602

patchstrbuf: move strbuf_readline_fd() from bundle.c to strbuf.{c,h}

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

## tl;dr

5 messages between Feb 11, 2012 and Feb 13, 2012. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

徐迪· Feb 11, 2012, 09:50 UTC · lore

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(-)
Show changes to 3 files +18 −17

bundle.c, strbuf.c, strbuf.h

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
徐迪· Feb 12, 2012, 12:25 UTC · re: 徐迪 · lore

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> 写道:
Show 90 quoted lines
> 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
Junio C Hamano· Feb 12, 2012, 17:07 UTC · re: 徐迪 · lore

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

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

徐迪· Feb 13, 2012, 03:54 UTC · re: Junio C Hamano · lore

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

Show 8 quoted lines
>> 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 :)

Show 7 quoted lines
>> 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?

Junio C Hamano· Feb 12, 2012, 17:20 UTC · re: 徐迪 · lore

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

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

← back to recent threads