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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 20, 2011, 09:31 UTC
Message-ID: <20110620093129.GB28282@elie>
In-Reply-To: <1308558173-29672-1-git-send-email-divanorama@gmail.com>

Hi,

Dmitry Ivankov wrote:

Show 6 quoted lines

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

Show 46 quoted lines

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