{"thread":{"id":"27661","subject":"Re: [PATCH svn-fe 1/7] Remove redundant buffer_fdinit calls","startedAt":"2011-06-20T09:31:29Z","lastAt":"2011-06-20T10:12:55Z","messageCount":2,"participants":["Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"170287","messageId":"20110620093129.GB28282@elie","threadId":"27661","inReplyTo":"1308558173-29672-1-git-send-email-divanorama@gmail.com","subject":"Re: [PATCH svn-fe 1/7] Remove redundant buffer_fdinit calls","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-20T09:31:29Z","receivedAt":"2011-06-20T09:31:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nDmitry Ivankov wrote:\n\n> First of all fast_export_init called buffer_fdinit directly,\n> and init_report buffer did fdopen once more - it is a FILE* leak.\n> The second problem is that fast_export_init used fd passed while\n> apply_delta used hardcoded REPORT_FILENO.\n>\n> Signed-off-by: Dmitry Ivankov \n\nGood eyes. As the patch \"vcs-svn: implement text-delta handling\"\nbounced forward to a newer codebase it seems we let report_buffer be\ninitialized twice. :/\n\nAbout the log message: it would be easier to read text that is (at\nleast mostly) wrapped to a consistent width.\n\nOkay, okay, more substantive comments. The ideal log message will\nexplain what the program currently does, why that's bad, and how the\npatch will improve it. In this example, that could mean something\nvaguely like:\n\n\tWhen importing from a dump with deltas, svn-fe's fast-export module\n\tcalls buffer_fdinit to initialize report_buffer twice: once in\n\tfast_export_init and once in init_report_buffer when processing the\n\tfirst delta. The second initialization is redundant and leaks a\n\tFILE *.\n\n\tFix it by relying on fast_export_init to initialize report_buffer.\n\tOn one hand explicitly initializing like this is simpler than the\n\ton-demand initialization implemented by init_report_buffer, and on\n\tthe other hand fast_export_init takes an fd as a parameter rather\n\tthan hard-coding the requirement to read from fd 3.\n\nExcept as noted above:\nReviewed-by: Jonathan Nieder \n\nBy the way, I think it would make sense to initialize \"postimage\" in\nfast_export_init (and deinitialize it in fast_export_deinit), too, for\na similar reason (simplification).\n\nThanks. Patch left unsnipped in case others have thoughts.\n\n> ---\n> I've added two more patches on top, they are less baked and so lack s-o-b.\n> \n> vcs-svn/fast_export.c | 12 ------------\n> 1 files changed, 0 insertions(+), 12 deletions(-)\n> \n> diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c\n> index 97f5fdf..3efde20 100644\n> --- a/vcs-svn/fast_export.c\n> +++ b/vcs-svn/fast_export.c\n> @@ -14,7 +14,6 @@\n> #include \"line_buffer.h\"\n> \n> #define MAX_GITSVN_LINE_LEN 4096\n> -#define REPORT_FILENO 3\n> \n> static uint32_t first_commit_done;\n> static struct line_buffer postimage = LINE_BUFFER_INIT;\n> @@ -30,15 +29,6 @@ static int init_postimage(void)\n> \treturn buffer_tmpfile_init(&postimage);\n> }\n> \n> -static int init_report_buffer(int fd)\n> -{\n> -\tstatic int report_buffer_initialized;\n> -\tif (report_buffer_initialized)\n> -\t\treturn 0;\n> -\treport_buffer_initialized = 1;\n> -\treturn buffer_fdinit(&report_buffer, fd);\n> -}\n> -\n> void fast_export_init(int fd)\n> {\n> \tif (buffer_fdinit(&report_buffer, fd))\n> @@ -203,8 +193,6 @@ static long apply_delta(off_t len, struct line_buffer *input,\n> \n> \tif (init_postimage() || !(out = buffer_tmpfile_rewind(&postimage)))\n> \t\tdie(\"cannot open temporary file for blob retrieval\");\n> -\tif (init_report_buffer(REPORT_FILENO))\n> -\t\tdie(\"cannot open fd 3 for feedback from fast-import\");\n> \tif (old_data) {\n> \t\tconst char *response;\n> \t\tprintf(\"cat-blob %s\\n\", old_data);\n> -- \n> 1.7.3.4\n> \n"},{"id":"170289","messageId":"20110620101254.GG28282@elie","threadId":"27661","inReplyTo":"1308558173-29672-5-git-send-email-divanorama@gmail.com","subject":"Re: [PATCH svn-fe 5/7] Fix vcs-svn/fast_export reinit bugs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-20T10:12:55Z","receivedAt":"2011-06-20T10:12:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nDmitry Ivankov wrote:\n\n> first_commit_done was not set in _init, but it\n> is needed if fast_export_ is used/_init-ed twice.\n\nThanks, good catch. I wonder if it's possible to detect this kind of\nthing automatically --- maybe it would make sense for test-svn-fe to\nlearn to apply two dumps in sequence so this code could be exercised.\n\n> Same thing for the branch_name.\n\nThis one should be squashed with the previous patch for easier review\nimho. New readers never have to know the details of early mistakes.\n\n>\n> Signed-off-by: Dmitry Ivankov \n> ---\n> vcs-svn/fast_export.c | 4 ++++\n> 1 files changed, 4 insertions(+), 0 deletions(-)\n>\n> diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c\n> index 8b14b74..6a4360f 100644\n> --- a/vcs-svn/fast_export.c\n> +++ b/vcs-svn/fast_export.c\n> @@ -32,6 +32,8 @@ static int init_postimage(void)\n> \n> void fast_export_init(int fd, const char* branch)\n> {\n> +\tfirst_commit_done = 0;\n> +\tstrbuf_reset(&branch_name);\n> \tstrbuf_addstr(&branch_name, branch);\n> \tif (buffer_fdinit(&report_buffer, fd))\n> \t\tdie_errno(\"cannot read from file descriptor %d\", fd);\n> @@ -45,7 +47,9 @@ void fast_export_deinit(void)\n> \n> void fast_export_reset(void)\n> {\n> +\tfirst_commit_done = 0;\n\nI don't think this is needed --- fast_export_reset is called when we\nare about to exit. Maybe the vcs-svn::*_reset functions should be\nrenamed to foo_free or something similar to avoid confusion\n(especially when comparing to strbuf_reset).\n\n> \tbuffer_reset(&report_buffer);\n> +\tstrbuf_reset(&branch_name);\n> }\n> \n> void fast_export_delete(const char *path)\n> -- \n> 1.7.3.4\n> \n"}]}