Re: [PATCH svn-fe 1/7] Remove redundant buffer_fdinit calls

Re: [PATCH svn-fe 1/7] Remove redundant buffer_fdinit calls

[Jonathan Nieder]

Hi,

Dmitry Ivankov wrote:

> First of all fast_export_init called buffer_fdinit directly,
> and init_report buffer did fdopen once more - it is a FILE* leak.
> The second problem is that fast_export_init used fd passed while
> apply_delta used hardcoded REPORT_FILENO.
>
> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>

Good eyes.  As the patch "vcs-svn: implement text-delta handling"
bounced forward to a newer codebase it seems we let report_buffer be
initialized twice. :/

About the log message: it would be easier to read text that is (at
least mostly) wrapped to a consistent width.

Okay, okay, more substantive comments.  The ideal log message will
explain what the program currently does, why that's bad, and how the
patch will improve it.  In this example, that could mean something
vaguely like:

	When importing from a dump with deltas, svn-fe's fast-export module
	calls buffer_fdinit to initialize report_buffer twice: once in
	fast_export_init and once in init_report_buffer when processing the
	first delta.  The second initialization is redundant and leaks a
	FILE *.

	Fix it by relying on fast_export_init to initialize report_buffer.
	On one hand explicitly initializing like this is simpler than the
	on-demand initialization implemented by init_report_buffer, and on
	the other hand fast_export_init takes an fd as a parameter rather
	than hard-coding the requirement to read from fd 3.

Except as noted above:
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

By the way, I think it would make sense to initialize "postimage" in
fast_export_init (and deinitialize it in fast_export_deinit), too, for
a similar reason (simplification).

Thanks.  Patch left unsnipped in case others have thoughts.

> ---
>  I've added two more patches on top, they are less baked and so lack s-o-b.
>  
>  vcs-svn/fast_export.c |   12 ------------
>  1 files changed, 0 insertions(+), 12 deletions(-)
> 
> diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
> index 97f5fdf..3efde20 100644
> --- a/vcs-svn/fast_export.c
> +++ b/vcs-svn/fast_export.c
> @@ -14,7 +14,6 @@
>  #include "line_buffer.h"
>  
>  #define MAX_GITSVN_LINE_LEN 4096
> -#define REPORT_FILENO 3
>  
>  static uint32_t first_commit_done;
>  static struct line_buffer postimage = LINE_BUFFER_INIT;
> @@ -30,15 +29,6 @@ static int init_postimage(void)
>  	return buffer_tmpfile_init(&postimage);
>  }
>  
> -static int init_report_buffer(int fd)
> -{
> -	static int report_buffer_initialized;
> -	if (report_buffer_initialized)
> -		return 0;
> -	report_buffer_initialized = 1;
> -	return buffer_fdinit(&report_buffer, fd);
> -}
> -
>  void fast_export_init(int fd)
>  {
>  	if (buffer_fdinit(&report_buffer, fd))
> @@ -203,8 +193,6 @@ static long apply_delta(off_t len, struct line_buffer *input,
>  
>  	if (init_postimage() || !(out = buffer_tmpfile_rewind(&postimage)))
>  		die("cannot open temporary file for blob retrieval");
> -	if (init_report_buffer(REPORT_FILENO))
> -		die("cannot open fd 3 for feedback from fast-import");
>  	if (old_data) {
>  		const char *response;
>  		printf("cat-blob %s\n", old_data);
> -- 
> 1.7.3.4
> 

Re: [PATCH svn-fe 5/7] Fix vcs-svn/fast_export reinit bugs

[Jonathan Nieder]

Hi,

Dmitry Ivankov wrote:

> first_commit_done was not set in _init, but it
> is needed if fast_export_ is used/_init-ed twice.

Thanks, good catch.  I wonder if it's possible to detect this kind of
thing automatically --- maybe it would make sense for test-svn-fe to
learn to apply two dumps in sequence so this code could be exercised.

> Same thing for the branch_name.

This one should be squashed with the previous patch for easier review
imho.  New readers never have to know the details of early mistakes.

>
> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
> ---
>  vcs-svn/fast_export.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
> index 8b14b74..6a4360f 100644
> --- a/vcs-svn/fast_export.c
> +++ b/vcs-svn/fast_export.c
> @@ -32,6 +32,8 @@ static int init_postimage(void)
>  
>  void fast_export_init(int fd, const char* branch)
>  {
> +	first_commit_done = 0;
> +	strbuf_reset(&branch_name);
>  	strbuf_addstr(&branch_name, branch);
>  	if (buffer_fdinit(&report_buffer, fd))
>  		die_errno("cannot read from file descriptor %d", fd);
> @@ -45,7 +47,9 @@ void fast_export_deinit(void)
>  
>  void fast_export_reset(void)
>  {
> +	first_commit_done = 0;

I don't think this is needed --- fast_export_reset is called when we
are about to exit.  Maybe the vcs-svn::*_reset functions should be
renamed to foo_free or something similar to avoid confusion
(especially when comparing to strbuf_reset).

>  	buffer_reset(&report_buffer);
> +	strbuf_reset(&branch_name);
>  }
>  
>  void fast_export_delete(const char *path)
> -- 
> 1.7.3.4
>