{"thread":{"id":"28218","subject":"[PATCH] vcs-svn: fix broken test 'keep content, but change mode'","startedAt":"2011-08-25T16:02:04Z","lastAt":"2011-08-25T16:08:26Z","messageCount":2,"participants":["Dmitry Ivankov"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"174233","messageId":"1314288124-16969-1-git-send-email-divanorama@gmail.com","threadId":"28218","inReplyTo":null,"subject":"[PATCH] vcs-svn: fix broken test 'keep content, but change mode'","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-08-25T16:02:04Z","receivedAt":"2011-08-25T16:02:04Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"svn symlinks are files with \"link destination\" content and a\n\"svn:special=*\" property set. These are imported as blobs with\n\"destination\" content and S_IFLNK mode. When svn copy a file without\naltering it's content(but maybe altering it's mode), we reuse the blob\nobject thus loosing or not adding the \"link \" prefix.\n\nBut we take possible prefix into account when applying svn deltas. And\nthis is the only place we ask fast-import for original blob. So pretend\nthat we want to apply a zero delta to resolve the issue.\n\nThere is some overhead due to using a temporary file to store such a small\nblob. But hopefully such node change is too rare to care.\n\nSigned-off-by: Dmitry Ivankov <divanorama@gmail.com>\n---\nThis could be done better if we just read the cat-blob to memory, added or\nremoved the \"link \" prefix and wrote it to the stream, because the link\ndestination should be a tiny string. But on the other hand it'd blow up\nif for some reason it's huge.\n\nAnd taking into account that changing file mode from/to link without a\ncontent change should be extremely rare anyway, I think it's ok.\n\nMaybe it is redundant to add svndiff0_identity function to just cat blob\nto a temporary file. The excuse is that svndiff.c is the only user of this\ntemporary file and the cat-blob response, so keep it there.\n\nThe patch base is svn-fe branch at git://repo.or.cz/git/jrn.git\nNot backporting it to git master because vcs-svn stuff differs quite much\naround this change.\n\n t/t9010-svn-fe.sh     |    2 +-\n vcs-svn/fast_export.c |    6 +++++-\n vcs-svn/svndiff.c     |   17 +++++++++++++++++\n vcs-svn/svndiff.h     |    1 +\n vcs-svn/svndump.c     |   16 +++++++++++++++-\n 5 files changed, 39 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh\nindex b7eed24..b6bdfeb 100755\n--- a/t/t9010-svn-fe.sh\n+++ b/t/t9010-svn-fe.sh\n@@ -413,7 +413,7 @@ test_expect_success PIPE 'action: add node without text' '\n \ttry_dump textless.dump must_fail\n '\n \n-test_expect_failure PIPE 'change file mode but keep old content' '\n+test_expect_success PIPE 'change file mode but keep old content' '\n \treinit_git &&\n \tcat >expect <<-\\EOF &&\n \tOBJID\ndiff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c\nindex 19d7c34..8c7295f 100644\n--- a/vcs-svn/fast_export.c\n+++ b/vcs-svn/fast_export.c\n@@ -209,7 +209,11 @@ static long apply_delta(off_t len, struct line_buffer *input,\n \t\tpreimage.max_off += strlen(\"link \");\n \t\tcheck_preimage_overflow(preimage.max_off, 1);\n \t}\n-\tif (svndiff0_apply(input, len, &preimage, out))\n+\n+\tif (!input) {\n+\t\tif (svndiff0_identity(&preimage, out))\n+\t\t\tdie(\"cannot cat blob\");\n+\t} else if (svndiff0_apply(input, len, &preimage, out))\n \t\tdie(\"cannot apply delta\");\n \tif (old_data) {\n \t\t/* Read the remainder of preimage and trailing newline. */\ndiff --git a/vcs-svn/svndiff.c b/vcs-svn/svndiff.c\nindex 9ee41bb..bf104db 100644\n--- a/vcs-svn/svndiff.c\n+++ b/vcs-svn/svndiff.c\n@@ -306,3 +306,20 @@ int svndiff0_apply(struct line_buffer *delta, off_t delta_len,\n \t}\n \treturn 0;\n }\n+int svndiff0_identity(struct sliding_view *preimage, FILE *postimage)\n+{\n+\tassert(preimage && postimage);\n+\toff_t pre_off = 0;\n+\n+\twhile (pre_off != preimage->max_off) {\n+\t\tsize_t pre_len = 8192;\n+\t\tif (pre_off + pre_len > preimage->max_off)\n+\t\t\tpre_len = preimage->max_off - pre_off;\n+\t\tif (move_window(preimage, pre_off, pre_len) ||\n+\t\t\twrite_strbuf(&preimage->buf, postimage))\n+\t\t\treturn -1;\n+\t\tpre_off += pre_len;\n+\t}\n+\n+\treturn 0;\n+}\ndiff --git a/vcs-svn/svndiff.h b/vcs-svn/svndiff.h\nindex 74eb464..5afa3f2 100644\n--- a/vcs-svn/svndiff.h\n+++ b/vcs-svn/svndiff.h\n@@ -6,5 +6,6 @@ struct sliding_view;\n \n extern int svndiff0_apply(struct line_buffer *delta, off_t delta_len,\n \t\tstruct sliding_view *preimage, FILE *postimage);\n+extern int svndiff0_identity(struct sliding_view *preimage, FILE *postimage);\n \n #endif\ndiff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c\nindex b1f4161..1e7ed48 100644\n--- a/vcs-svn/svndump.c\n+++ b/vcs-svn/svndump.c\n@@ -285,7 +285,21 @@ static void handle_node(void)\n \t\t/* For the fast_export_* functions, NULL means empty. */\n \t\told_data = NULL;\n \tif (!have_text) {\n-\t\tfast_export_modify(node_ctx.dst.buf, node_ctx.type, old_data);\n+\t\t/*\n+\t\t * This is clean content copy in svn, but we alter the content\n+\t\t * of symlinks (add/remove \"link \" prefix used by svn). So when\n+\t\t * mode changes from/to symlink specify (recreate) data inline.\n+\t\t */\n+\t\tif (node_ctx.type != old_mode && (old_mode == REPO_MODE_LNK\n+\t\t\t\t\t|| node_ctx.type == REPO_MODE_LNK)) {\n+\n+\t\t\tfast_export_modify(node_ctx.dst.buf,\n+\t\t\t\t\t\tnode_ctx.type, \"inline\");\n+\t\t\tfast_export_blob_delta(node_ctx.type, old_mode,\n+\t\t\t\t\t\told_data, 0, NULL);\n+\t\t} else\n+\t\t\tfast_export_modify(node_ctx.dst.buf,\n+\t\t\t\t\t\tnode_ctx.type, old_data);\n \t\treturn;\n \t}\n \tif (!node_ctx.text_delta) {\n-- \n1.7.3.4\n"},{"id":"174236","messageId":"CA+gfSn8a7kjbQFP0A2BHfro8MOhpY-TxDr5Fr+=0qtD9O62GPA@mail.gmail.com","threadId":"28218","inReplyTo":"1314288124-16969-1-git-send-email-divanorama@gmail.com","subject":"Re: [PATCH] vcs-svn: fix broken test 'keep content, but change mode'","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-08-25T16:08:26Z","receivedAt":"2011-08-25T16:08:26Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"correct David's email (on first attempt I've accidentally used the old\none, taken from some git history)\n\nOn Thu, Aug 25, 2011 at 10:02 PM, Dmitry Ivankov <divanorama@gmail.com> wrote:\n> svn symlinks are files with \"link destination\" content and a\n> \"svn:special=*\" property set. These are imported as blobs with\n> \"destination\" content and S_IFLNK mode. When svn copy a file without\n> altering it's content(but maybe altering it's mode), we reuse the blob\n> object thus loosing or not adding the \"link \" prefix.\n>\n> But we take possible prefix into account when applying svn deltas. And\n> this is the only place we ask fast-import for original blob. So pretend\n> that we want to apply a zero delta to resolve the issue.\n>\n> There is some overhead due to using a temporary file to store such a small\n> blob. But hopefully such node change is too rare to care.\n>\n> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>\n> ---\n> This could be done better if we just read the cat-blob to memory, added or\n> removed the \"link \" prefix and wrote it to the stream, because the link\n> destination should be a tiny string. But on the other hand it'd blow up\n> if for some reason it's huge.\n>\n> And taking into account that changing file mode from/to link without a\n> content change should be extremely rare anyway, I think it's ok.\n>\n> Maybe it is redundant to add svndiff0_identity function to just cat blob\n> to a temporary file. The excuse is that svndiff.c is the only user of this\n> temporary file and the cat-blob response, so keep it there.\n>\n> The patch base is svn-fe branch at git://repo.or.cz/git/jrn.git\n> Not backporting it to git master because vcs-svn stuff differs quite much\n> around this change.\n>\n>  t/t9010-svn-fe.sh     |    2 +-\n>  vcs-svn/fast_export.c |    6 +++++-\n>  vcs-svn/svndiff.c     |   17 +++++++++++++++++\n>  vcs-svn/svndiff.h     |    1 +\n>  vcs-svn/svndump.c     |   16 +++++++++++++++-\n>  5 files changed, 39 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh\n> index b7eed24..b6bdfeb 100755\n> --- a/t/t9010-svn-fe.sh\n> +++ b/t/t9010-svn-fe.sh\n> @@ -413,7 +413,7 @@ test_expect_success PIPE 'action: add node without text' '\n>        try_dump textless.dump must_fail\n>  '\n>\n> -test_expect_failure PIPE 'change file mode but keep old content' '\n> +test_expect_success PIPE 'change file mode but keep old content' '\n>        reinit_git &&\n>        cat >expect <<-\\EOF &&\n>        OBJID\n> diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c\n> index 19d7c34..8c7295f 100644\n> --- a/vcs-svn/fast_export.c\n> +++ b/vcs-svn/fast_export.c\n> @@ -209,7 +209,11 @@ static long apply_delta(off_t len, struct line_buffer *input,\n>                preimage.max_off += strlen(\"link \");\n>                check_preimage_overflow(preimage.max_off, 1);\n>        }\n> -       if (svndiff0_apply(input, len, &preimage, out))\n> +\n> +       if (!input) {\n> +               if (svndiff0_identity(&preimage, out))\n> +                       die(\"cannot cat blob\");\n> +       } else if (svndiff0_apply(input, len, &preimage, out))\n>                die(\"cannot apply delta\");\n>        if (old_data) {\n>                /* Read the remainder of preimage and trailing newline. */\n> diff --git a/vcs-svn/svndiff.c b/vcs-svn/svndiff.c\n> index 9ee41bb..bf104db 100644\n> --- a/vcs-svn/svndiff.c\n> +++ b/vcs-svn/svndiff.c\n> @@ -306,3 +306,20 @@ int svndiff0_apply(struct line_buffer *delta, off_t delta_len,\n>        }\n>        return 0;\n>  }\n> +int svndiff0_identity(struct sliding_view *preimage, FILE *postimage)\n> +{\n> +       assert(preimage && postimage);\n> +       off_t pre_off = 0;\n> +\n> +       while (pre_off != preimage->max_off) {\n> +               size_t pre_len = 8192;\n> +               if (pre_off + pre_len > preimage->max_off)\n> +                       pre_len = preimage->max_off - pre_off;\n> +               if (move_window(preimage, pre_off, pre_len) ||\n> +                       write_strbuf(&preimage->buf, postimage))\n> +                       return -1;\n> +               pre_off += pre_len;\n> +       }\n> +\n> +       return 0;\n> +}\n> diff --git a/vcs-svn/svndiff.h b/vcs-svn/svndiff.h\n> index 74eb464..5afa3f2 100644\n> --- a/vcs-svn/svndiff.h\n> +++ b/vcs-svn/svndiff.h\n> @@ -6,5 +6,6 @@ struct sliding_view;\n>\n>  extern int svndiff0_apply(struct line_buffer *delta, off_t delta_len,\n>                struct sliding_view *preimage, FILE *postimage);\n> +extern int svndiff0_identity(struct sliding_view *preimage, FILE *postimage);\n>\n>  #endif\n> diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c\n> index b1f4161..1e7ed48 100644\n> --- a/vcs-svn/svndump.c\n> +++ b/vcs-svn/svndump.c\n> @@ -285,7 +285,21 @@ static void handle_node(void)\n>                /* For the fast_export_* functions, NULL means empty. */\n>                old_data = NULL;\n>        if (!have_text) {\n> -               fast_export_modify(node_ctx.dst.buf, node_ctx.type, old_data);\n> +               /*\n> +                * This is clean content copy in svn, but we alter the content\n> +                * of symlinks (add/remove \"link \" prefix used by svn). So when\n> +                * mode changes from/to symlink specify (recreate) data inline.\n> +                */\n> +               if (node_ctx.type != old_mode && (old_mode == REPO_MODE_LNK\n> +                                       || node_ctx.type == REPO_MODE_LNK)) {\n> +\n> +                       fast_export_modify(node_ctx.dst.buf,\n> +                                               node_ctx.type, \"inline\");\n> +                       fast_export_blob_delta(node_ctx.type, old_mode,\n> +                                               old_data, 0, NULL);\n> +               } else\n> +                       fast_export_modify(node_ctx.dst.buf,\n> +                                               node_ctx.type, old_data);\n>                return;\n>        }\n>        if (!node_ctx.text_delta) {\n> --\n> 1.7.3.4\n>\n>\n"}]}