From: David Michael Barr <davidbarr@google.com>
Date: Tue, 05 Jun 2012 01:21:05 GMT
Subject: Re: [RFC 3/4] Add svndump_init_fd to allow reading dumps from arbitrary FDs.
Message-ID: <CAFfmPPO3Yvbs0EzMQa2z13ERi6Ocjv3W=8FYaAGPt739bD_qbw@mail.gmail.com>
In-Reply-To: <1338830455-3091-4-git-send-email-florian.achleitner.2.6.31@gmail.com>

On Tue, Jun 5, 2012 at 3:20 AM, Florian Achleitner
<florian.achleitner.2.6.31@gmail.com> wrote:
> The existing function only allowed reading from a filename or
> from stdin.
>
> Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> ---
>  vcs-svn/svndump.c |   20 +++++++++++++++++---
>  vcs-svn/svndump.h |    1 +
>  2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
> index 0899790..2f0089f 100644
> --- a/vcs-svn/svndump.c
> +++ b/vcs-svn/svndump.c
> @@ -465,10 +465,8 @@ void svndump_read(const char *url)
>                end_revision();
>  }
>
> -int svndump_init(const char *filename)
> +static void init()
>  {
> -       if (buffer_init(&input, filename))
> -               return error("cannot open %s: %s", filename, strerror(errno));
>        fast_export_init(REPORT_FILENO);
>        strbuf_init(&dump_ctx.uuid, 4096);
>        strbuf_init(&dump_ctx.url, 4096);
> @@ -479,6 +477,22 @@ int svndump_init(const char *filename)
>        reset_dump_ctx(NULL);
>        reset_rev_ctx(0);
>        reset_node_ctx(NULL);
> +       return;
> +}
> +
> +int svndump_init(const char *filename)
> +{
> +       if (buffer_init(&input, filename))
> +               return error("cannot open %s: %s", filename, strerror(errno));

Note: filename is allowed to be NULL here.
This is a bug in the existing code that you just moved.

I suggest moving error printing into buffer_init().
This way the basis for the message is clearer.

For bonus points, we should split buffer_init().
Plain buffer_init() should use stdin.
The new buff_init_path() should take a filename.

> +       init();
> +       return 0;
> +}
> +
> +int svndump_init_fd(int in_fd)
> +{
> +       if(buffer_fdinit(&input, in_fd))
> +               return error("cannot open fd %d: %s", in_fd, strerror(errno));
> +       init();
>        return 0;
>  }
>
> diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h
> index df9ceb0..24e7beb 100644
> --- a/vcs-svn/svndump.h
> +++ b/vcs-svn/svndump.h
> @@ -2,6 +2,7 @@
>  #define SVNDUMP_H_
>
>  int svndump_init(const char *filename);
> +int svndump_init_fd(int in_fd);
>  void svndump_read(const char *url);
>  void svndump_deinit(void);
>  void svndump_reset(void);
> --
> 1.7.9.5

Otherwise, I like the direction of this patch.

--
David Barr