threads / patch / 28218

patchvcs-svn: fix broken test 'keep content, but change mode'

Subject: [PATCH] vcs-svn: fix broken test 'keep content, but change mode'

## tl;dr

2 messages between Aug 25, 2011 and Aug 25, 2011. Diffs are folded; open one to read it.

replies: 1people: 1as markdown or json

Dmitry Ivankov· Aug 25, 2011, 16:02 UTC · lore

svn symlinks are files with "link destination" content and a "svn:special=*" property set. These are imported as blobs with "destination" content and S_IFLNK mode. When svn copy a file without altering it's content(but maybe altering it's mode), we reuse the blob object thus loosing or not adding the "link " prefix.

But we take possible prefix into account when applying svn deltas. And this is the only place we ask fast-import for original blob. So pretend that we want to apply a zero delta to resolve the issue.

There is some overhead due to using a temporary file to store such a small blob. But hopefully such node change is too rare to care.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
This could be done better if we just read the cat-blob to memory, added or
removed the "link " prefix and wrote it to the stream, because the link
destination should be a tiny string. But on the other hand it'd blow up
if for some reason it's huge.

And taking into account that changing file mode from/to link without a content change should be extremely rare anyway, I think it's ok.

Maybe it is redundant to add svndiff0_identity function to just cat blob to a temporary file. The excuse is that svndiff.c is the only user of this temporary file and the cat-blob response, so keep it there.

The patch base is svn-fe branch at git://repo.or.cz/git/jrn.git Not backporting it to git master because vcs-svn stuff differs quite much around this change.

 t/t9010-svn-fe.sh     |    2 +-
 vcs-svn/fast_export.c |    6 +++++-
 vcs-svn/svndiff.c     |   17 +++++++++++++++++
 vcs-svn/svndiff.h     |    1 +
 vcs-svn/svndump.c     |   16 +++++++++++++++-
 5 files changed, 39 insertions(+), 3 deletions(-)
Show changes to 5 files +39 −3

t/t9010-svn-fe.sh, vcs-svn/fast_export.c, vcs-svn/svndiff.c, vcs-svn/svndiff.h, vcs-svn/svndump.c

diff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh
index b7eed24..b6bdfeb 100755
--- a/t/t9010-svn-fe.sh
+++ b/t/t9010-svn-fe.sh
@@ -413,7 +413,7 @@ test_expect_success PIPE 'action: add node without text' '
 	try_dump textless.dump must_fail
 '
 
-test_expect_failure PIPE 'change file mode but keep old content' '
+test_expect_success PIPE 'change file mode but keep old content' '
 	reinit_git &&
 	cat >expect <<-\EOF &&
 	OBJID
diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 19d7c34..8c7295f 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -209,7 +209,11 @@ static long apply_delta(off_t len, struct line_buffer *input,
 		preimage.max_off += strlen("link ");
 		check_preimage_overflow(preimage.max_off, 1);
 	}
-	if (svndiff0_apply(input, len, &preimage, out))
+
+	if (!input) {
+		if (svndiff0_identity(&preimage, out))
+			die("cannot cat blob");
+	} else if (svndiff0_apply(input, len, &preimage, out))
 		die("cannot apply delta");
 	if (old_data) {
 		/* Read the remainder of preimage and trailing newline. */
diff --git a/vcs-svn/svndiff.c b/vcs-svn/svndiff.c
index 9ee41bb..bf104db 100644
--- a/vcs-svn/svndiff.c
+++ b/vcs-svn/svndiff.c
@@ -306,3 +306,20 @@ int svndiff0_apply(struct line_buffer *delta, off_t delta_len,
 	}
 	return 0;
 }
+int svndiff0_identity(struct sliding_view *preimage, FILE *postimage)
+{
+	assert(preimage && postimage);
+	off_t pre_off = 0;
+
+	while (pre_off != preimage->max_off) {
+		size_t pre_len = 8192;
+		if (pre_off + pre_len > preimage->max_off)
+			pre_len = preimage->max_off - pre_off;
+		if (move_window(preimage, pre_off, pre_len) ||
+			write_strbuf(&preimage->buf, postimage))
+			return -1;
+		pre_off += pre_len;
+	}
+
+	return 0;
+}
diff --git a/vcs-svn/svndiff.h b/vcs-svn/svndiff.h
index 74eb464..5afa3f2 100644
--- a/vcs-svn/svndiff.h
+++ b/vcs-svn/svndiff.h
@@ -6,5 +6,6 @@ struct sliding_view;
 
 extern int svndiff0_apply(struct line_buffer *delta, off_t delta_len,
 		struct sliding_view *preimage, FILE *postimage);
+extern int svndiff0_identity(struct sliding_view *preimage, FILE *postimage);
 
 #endif
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index b1f4161..1e7ed48 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -285,7 +285,21 @@ static void handle_node(void)
 		/* For the fast_export_* functions, NULL means empty. */
 		old_data = NULL;
 	if (!have_text) {
-		fast_export_modify(node_ctx.dst.buf, node_ctx.type, old_data);
+		/*
+		 * This is clean content copy in svn, but we alter the content
+		 * of symlinks (add/remove "link " prefix used by svn). So when
+		 * mode changes from/to symlink specify (recreate) data inline.
+		 */
+		if (node_ctx.type != old_mode && (old_mode == REPO_MODE_LNK
+					|| node_ctx.type == REPO_MODE_LNK)) {
+
+			fast_export_modify(node_ctx.dst.buf,
+						node_ctx.type, "inline");
+			fast_export_blob_delta(node_ctx.type, old_mode,
+						old_data, 0, NULL);
+		} else
+			fast_export_modify(node_ctx.dst.buf,
+						node_ctx.type, old_data);
 		return;
 	}
 	if (!node_ctx.text_delta) {
-- 
1.7.3.4
Dmitry Ivankov· Aug 25, 2011, 16:08 UTC · re: Dmitry Ivankov · lore

Re: [PATCH] vcs-svn: fix broken test 'keep content, but change mode'

correct David's email (on first attempt I've accidentally used the old one, taken from some git history)

On Thu, Aug 25, 2011 at 10:02 PM, Dmitry Ivankov <divanorama@gmail.com> wrote:
Show 135 quoted lines
> svn symlinks are files with "link destination" content and a
> "svn:special=*" property set. These are imported as blobs with
> "destination" content and S_IFLNK mode. When svn copy a file without
> altering it's content(but maybe altering it's mode), we reuse the blob
> object thus loosing or not adding the "link " prefix.
>
> But we take possible prefix into account when applying svn deltas. And
> this is the only place we ask fast-import for original blob. So pretend
> that we want to apply a zero delta to resolve the issue.
>
> There is some overhead due to using a temporary file to store such a small
> blob. But hopefully such node change is too rare to care.
>
> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
> ---
> This could be done better if we just read the cat-blob to memory, added or
> removed the "link " prefix and wrote it to the stream, because the link
> destination should be a tiny string. But on the other hand it'd blow up
> if for some reason it's huge.
>
> And taking into account that changing file mode from/to link without a
> content change should be extremely rare anyway, I think it's ok.
>
> Maybe it is redundant to add svndiff0_identity function to just cat blob
> to a temporary file. The excuse is that svndiff.c is the only user of this
> temporary file and the cat-blob response, so keep it there.
>
> The patch base is svn-fe branch at git://repo.or.cz/git/jrn.git
> Not backporting it to git master because vcs-svn stuff differs quite much
> around this change.
>
>  t/t9010-svn-fe.sh     |    2 +-
>  vcs-svn/fast_export.c |    6 +++++-
>  vcs-svn/svndiff.c     |   17 +++++++++++++++++
>  vcs-svn/svndiff.h     |    1 +
>  vcs-svn/svndump.c     |   16 +++++++++++++++-
>  5 files changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh
> index b7eed24..b6bdfeb 100755
> --- a/t/t9010-svn-fe.sh
> +++ b/t/t9010-svn-fe.sh
> @@ -413,7 +413,7 @@ test_expect_success PIPE 'action: add node without text' '
>        try_dump textless.dump must_fail
>  '
>
> -test_expect_failure PIPE 'change file mode but keep old content' '
> +test_expect_success PIPE 'change file mode but keep old content' '
>        reinit_git &&
>        cat >expect <<-\EOF &&
>        OBJID
> diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
> index 19d7c34..8c7295f 100644
> --- a/vcs-svn/fast_export.c
> +++ b/vcs-svn/fast_export.c
> @@ -209,7 +209,11 @@ static long apply_delta(off_t len, struct line_buffer *input,
>                preimage.max_off += strlen("link ");
>                check_preimage_overflow(preimage.max_off, 1);
>        }
> -       if (svndiff0_apply(input, len, &preimage, out))
> +
> +       if (!input) {
> +               if (svndiff0_identity(&preimage, out))
> +                       die("cannot cat blob");
> +       } else if (svndiff0_apply(input, len, &preimage, out))
>                die("cannot apply delta");
>        if (old_data) {
>                /* Read the remainder of preimage and trailing newline. */
> diff --git a/vcs-svn/svndiff.c b/vcs-svn/svndiff.c
> index 9ee41bb..bf104db 100644
> --- a/vcs-svn/svndiff.c
> +++ b/vcs-svn/svndiff.c
> @@ -306,3 +306,20 @@ int svndiff0_apply(struct line_buffer *delta, off_t delta_len,
>        }
>        return 0;
>  }
> +int svndiff0_identity(struct sliding_view *preimage, FILE *postimage)
> +{
> +       assert(preimage && postimage);
> +       off_t pre_off = 0;
> +
> +       while (pre_off != preimage->max_off) {
> +               size_t pre_len = 8192;
> +               if (pre_off + pre_len > preimage->max_off)
> +                       pre_len = preimage->max_off - pre_off;
> +               if (move_window(preimage, pre_off, pre_len) ||
> +                       write_strbuf(&preimage->buf, postimage))
> +                       return -1;
> +               pre_off += pre_len;
> +       }
> +
> +       return 0;
> +}
> diff --git a/vcs-svn/svndiff.h b/vcs-svn/svndiff.h
> index 74eb464..5afa3f2 100644
> --- a/vcs-svn/svndiff.h
> +++ b/vcs-svn/svndiff.h
> @@ -6,5 +6,6 @@ struct sliding_view;
>
>  extern int svndiff0_apply(struct line_buffer *delta, off_t delta_len,
>                struct sliding_view *preimage, FILE *postimage);
> +extern int svndiff0_identity(struct sliding_view *preimage, FILE *postimage);
>
>  #endif
> diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
> index b1f4161..1e7ed48 100644
> --- a/vcs-svn/svndump.c
> +++ b/vcs-svn/svndump.c
> @@ -285,7 +285,21 @@ static void handle_node(void)
>                /* For the fast_export_* functions, NULL means empty. */
>                old_data = NULL;
>        if (!have_text) {
> -               fast_export_modify(node_ctx.dst.buf, node_ctx.type, old_data);
> +               /*
> +                * This is clean content copy in svn, but we alter the content
> +                * of symlinks (add/remove "link " prefix used by svn). So when
> +                * mode changes from/to symlink specify (recreate) data inline.
> +                */
> +               if (node_ctx.type != old_mode && (old_mode == REPO_MODE_LNK
> +                                       || node_ctx.type == REPO_MODE_LNK)) {
> +
> +                       fast_export_modify(node_ctx.dst.buf,
> +                                               node_ctx.type, "inline");
> +                       fast_export_blob_delta(node_ctx.type, old_mode,
> +                                               old_data, 0, NULL);
> +               } else
> +                       fast_export_modify(node_ctx.dst.buf,
> +                                               node_ctx.type, old_data);
>                return;
>        }
>        if (!node_ctx.text_delta) {
> --
> 1.7.3.4
>
>

← back to recent threads