You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Blair Zajac <bl...@orcaware.com> on 2008/06/06 15:20:36 UTC
File externals and wc style questions
I'm working on getting intra-repository file externals into svn's wc, as an
addition to our support for directory externals.
A couple of design questions, but first a design point. I'm implementing file
externals using the existing support in 1.5 that you can do 'touch foo; svn add
foo; svn switch foo URL'. So under the hood, having a file external sets up a
switched file. The existence of some additional fields in the entries will tell
the client that it still needs to be treated specially.
Because the svn:externals property can add an external to subdirectories and
users may do updates from a subdirectory, svn may not know that a switched path
is a file external, so I need to add a marker in the wc entries that something
is a file external and not just a switched path.
So I can add a single entry line into .svn/entries with a single line, either of
the two following form:
[HEAD|r\d+] URL
or two separate lines:
URL
[HEAD|r\d+]
The two have to both be there for this to work, so maybe the first?
Also, representing a HEAD or fixed revision number the svn_opt_revision_t seems
natural for this, but this has a number of different revision types that I don't
need. Should I have a new smaller union type that just reflects a HEAD revision
and a fixed revision?
Any style feedback welcome.
Patch for two separate lines in .svn/entries is below.
Oh yes, and why do we have svn_wc__atts_to_entry()? This looks like it's more
XML entries files related?
Blair
Index: subversion/include/svn_wc.h
===================================================================
--- subversion/include/svn_wc.h (revision 31613)
+++ subversion/include/svn_wc.h (working copy)
@@ -1861,6 +1861,23 @@
* @since New in 1.5. */
svn_depth_t depth;
+ /** The entry is a file external and this is the repository root
+ * relative path to the file, otherwise NULL if the file is not a
+ * file external.
+ *
+ * @since New in 1.6. */
+ const char *file_external_path;
+
+ /** The entry is a file external and this is the revision number
+ * that the external is checked out from. The only permissable
+ * values are svn_opt_revision_unspecified if the entry is not an
+ * external, svn_opt_revision_head if the external revision is
+ * unspecified or specified with -r HEAD or svn_opt_revision_number
+ * for a specifiec revision number.
+ *
+ * @since New in 1.6. */
+ svn_opt_revision_t file_external_rev;
+
/* IMPORTANT: If you extend this structure, check the following functions in
* subversion/libsvn_wc/entries.c, to see if you need to extend them as well.
*
Index: subversion/libsvn_wc/entries.c
===================================================================
--- subversion/libsvn_wc/entries.c (revision 31613)
+++ subversion/libsvn_wc/entries.c (working copy)
@@ -2,7 +2,7 @@
* entries.c : manipulating the administrative `entries' file.
*
* ====================================================================
- * Copyright (c) 2000-2007 CollabNet. All rights reserved.
+ * Copyright (c) 2000-2008 CollabNet. All rights reserved.
*
* This software is licensed as described in the file COPYING, which
* you should have received as part of this distribution. The terms
@@ -292,6 +292,41 @@
return SVN_NO_ERROR;
}
+/* Parse a file external revision number from a NULL terminated string
+ REV_STR into *RESULT. Valid strings are NULL, "HEAD", or r\d+. A
+ NULL string converts to a HEAD revision. */
+static svn_error_t *
+parse_file_external_rev(svn_opt_revision_t *result, const char *rev_str,
+ const char *name)
+{
+ svn_opt_revision_t r;
+
+ if (! rev_str)
+ r.kind = svn_opt_revision_head;
+ else
+ {
+ if (strcmp(rev_str, SVN_WC__ENTRY_VALUE_HEAD) == 0)
+ {
+ }
+ else if (rev_str[0] == 'r')
+ {
+ svn_revnum_t rev;
+ SVN_ERR(svn_revnum_parse(&rev, rev_str+1, NULL));
+ r.kind = svn_opt_revision_number;
+ r.value.number = rev;
+ }
+ else
+ return svn_error_createf(SVN_ERR_WC_CORRUPT, NULL,
+ _("Entry for '%s' has invalid file external "
+ "revision '%s'"),
+ name ? name : SVN_WC_ENTRY_THIS_DIR,
+ rev_str);
+ }
+ *result = r;
+
+ return SVN_NO_ERROR;
+}
+
/* Allocate an entry from POOL and read it from [*BUF, END). The
buffer may be modified in place while parsing. Return the new
entry in *NEW_ENTRY. Advance *BUF to point at the end of the entry
@@ -498,6 +533,22 @@
}
MAYBE_DONE;
+ /* File external relative path. */
+ SVN_ERR(read_str(&entry->file_external_path, buf, end, pool));
+ MAYBE_DONE;
+
+ /* File external revision. */
+ {
+ const char *rev_str;
+
+ entry->file_external_rev.kind = svn_opt_revision_head;
+
+ SVN_ERR(read_str(&rev_str, buf, end, pool));
+ SVN_ERR(parse_file_external_rev(&(entry->file_external_rev), rev_str,
+ name));
+ }
+ MAYBE_DONE;
+
done:
*new_entry = entry;
return SVN_NO_ERROR;
@@ -911,6 +962,30 @@
}
}
+ /* File externals */
+ entry->file_external_path =
+ apr_hash_get(atts, SVN_WC__ENTRY_ATTR_FILE_EXTERNAL_PATH,
+ APR_HASH_KEY_STRING);
+ if (entry->file_external_path)
+ {
+ *modify_flags |= SVN_WC__ENTRY_MODIFY_FILE_EXTERNAL_PATH;
+ entry->file_external_path = apr_pstrdup(pool, entry->file_external_path);
+ }
+
+ {
+ const char *rev_str;
+
+ rev_str = apr_hash_get(atts, SVN_WC__ENTRY_ATTR_FILE_EXTERNAL_REV,
+ APR_HASH_KEY_STRING);
+ if (rev_str)
+ {
+ SVN_ERR(parse_file_external_rev(&(entry->file_external_rev),
+ rev_str,
+ NULL));
+ *modify_flags |= SVN_WC__ENTRY_MODIFY_FILE_EXTERNAL_REV;
+ }
+ }
+
*new_entry = entry;
return SVN_NO_ERROR;
}
@@ -1664,6 +1739,27 @@
write_val(buf, val, strlen(val));
}
+ /* File externals. */
+ write_str(buf, entry->file_external_path, pool);
+ switch (entry->file_external_rev.kind)
+ {
+ case svn_opt_revision_head:
+ write_val(buf, SVN_WC__ENTRY_VALUE_HEAD,
+ sizeof(SVN_WC__ENTRY_VALUE_HEAD) - 1);
+ break;
+ case svn_opt_revision_number:
+ if (SVN_IS_VALID_REVNUM(entry->file_external_rev.value.number))
+ {
+ svn_stringbuf_appendbytes(buf, "r", 1);
+ write_revnum(buf, entry->file_external_rev.value.number, pool);
+ }
+ else
+ write_val(buf, NULL, 0);
+ break;
+ default:
+ write_val(buf, NULL, 0);
+ }
+
/* Remove redundant separators at the end of the entry. */
while (buf->len > 1 && buf->data[buf->len - 2] == '\n')
buf->len--;
@@ -2311,6 +2407,16 @@
cur_entry->keep_local = FALSE;
}
+ /* File externals. */
+ if (modify_flags & SVN_WC__ENTRY_MODIFY_FILE_EXTERNAL_PATH)
+ cur_entry->file_external_path = (entry->file_external_path
+ ? apr_pstrdup(pool,
+ entry->file_external_path)
+ : NULL);
+
+ if (modify_flags & SVN_WC__ENTRY_MODIFY_FILE_EXTERNAL_REV)
+ cur_entry->file_external_rev = entry->file_external_rev;
+
/* Make sure the entry exists in the entries hash. Possibly it
already did, in which case this could have been skipped, but what
the heck. */
@@ -2675,6 +2781,9 @@
dupentry->cachable_props = apr_pstrdup(pool, entry->cachable_props);
if (entry->present_props)
dupentry->present_props = apr_pstrdup(pool, entry->present_props);
+ if (entry->file_external_path)
+ dupentry->file_external_path = apr_pstrdup(pool,
+ entry->file_external_path);
return dupentry;
}
Index: subversion/libsvn_wc/wc.h
===================================================================
--- subversion/libsvn_wc/wc.h (revision 31613)
+++ subversion/libsvn_wc/wc.h (working copy)
@@ -2,7 +2,7 @@
* wc.h : shared stuff internal to the svn_wc library.
*
* ====================================================================
- * Copyright (c) 2000-2007 CollabNet. All rights reserved.
+ * Copyright (c) 2000-2008 CollabNet. All rights reserved.
*
* This software is licensed as described in the file COPYING, which
* you should have received as part of this distribution. The terms
@@ -69,9 +69,11 @@
* The change from 8 to 9 was the addition of changelists, keep-local,
* and sticky depth (for selective/sparse checkouts).
*
+ * The change from 9 to 10 was the addition of file externals.
+ *
* Please document any further format changes here.
*/
-#define SVN_WC__VERSION 9
+#define SVN_WC__VERSION 10
/* A version <= this doesn't have property caching in the entries file. */
#define SVN_WC__NO_PROPCACHING_VERSION 5
Index: subversion/libsvn_wc/entries.h
===================================================================
--- subversion/libsvn_wc/entries.h (revision 31613)
+++ subversion/libsvn_wc/entries.h (working copy)
@@ -77,12 +77,16 @@
#define SVN_WC__ENTRY_ATTR_CHANGELIST "changelist"
#define SVN_WC__ENTRY_ATTR_KEEP_LOCAL "keep-local"
#define SVN_WC__ENTRY_ATTR_WORKING_SIZE "working-size"
+#define SVN_WC__ENTRY_ATTR_FILE_EXTERNAL_PATH "file-external-path"
+#define SVN_WC__ENTRY_ATTR_FILE_EXTERNAL_REV "file-external-rev"
/* Attribute values for 'schedule' */
#define SVN_WC__ENTRY_VALUE_ADD "add"
#define SVN_WC__ENTRY_VALUE_DELETE "delete"
#define SVN_WC__ENTRY_VALUE_REPLACE "replace"
+/* Attribute value for 'file-external' */
+#define SVN_WC__ENTRY_VALUE_HEAD "head"
/* Initialize an entries file based on URL at INITIAL_REV, in the adm
@@ -159,6 +163,8 @@
#define SVN_WC__ENTRY_MODIFY_CHANGELIST APR_INT64_C(0x0000000040000000)
#define SVN_WC__ENTRY_MODIFY_KEEP_LOCAL APR_INT64_C(0x0000000080000000)
#define SVN_WC__ENTRY_MODIFY_WORKING_SIZE APR_INT64_C(0x0000000100000000)
+#define SVN_WC__ENTRY_MODIFY_FILE_EXTERNAL_PATH APR_INT64_C(0x0000000200000000)
+#define SVN_WC__ENTRY_MODIFY_FILE_EXTERNAL_REV APR_INT64_C(0x0000000400000000)
/* No #define for DEPTH, because it's only meaningful on this-dir anyway. */
/* ...ORed together with this to mean "I really mean this, don't be
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: File externals and wc style questions
Posted by Blair Zajac <bl...@orcaware.com>.
David Glasser wrote:
> On Fri, Jun 6, 2008 at 8:20 AM, Blair Zajac <bl...@orcaware.com> wrote:
>> I'm working on getting intra-repository file externals into svn's wc, as an
>> addition to our support for directory externals.
>>
>> A couple of design questions, but first a design point. I'm implementing
>> file externals using the existing support in 1.5 that you can do 'touch foo;
>> svn add foo; svn switch foo URL'. So under the hood, having a file external
>> sets up a switched file. The existence of some additional fields in the
>> entries will tell the client that it still needs to be treated specially.
>
> I haven't completely thought through exactly what you're saying, but
> anything that makes "externals" more like "repository-defined
> suggestions for switches" is great in my book.
>
>> Because the svn:externals property can add an external to subdirectories and
>> users may do updates from a subdirectory, svn may not know that a switched
>> path is a file external, so I need to add a marker in the wc entries that
>> something is a file external and not just a switched path.
>>
>> So I can add a single entry line into .svn/entries with a single line,
>> either of the two following form:
>>
>> [HEAD|r\d+] URL
>>
>> or two separate lines:
>>
>> URL
>> [HEAD|r\d+]
>>
>> The two have to both be there for this to work, so maybe the first?
Yes, I'm going to switch to that.
> Parsing sucks. Later we'll decide that you can support, say, the
> {DATE} revision type, which can have a space in it. Be less
> ambiguous.
I'm not going to have {DATE} in this line. The svn:externals property may only
have r\d+ and even if we allowed {DATE} in the svn:externals, I would still
resolve this to a revision for the .svn/entries file.
Blair
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: File externals and wc style questions
Posted by David Glasser <gl...@davidglasser.net>.
On Fri, Jun 6, 2008 at 8:20 AM, Blair Zajac <bl...@orcaware.com> wrote:
> I'm working on getting intra-repository file externals into svn's wc, as an
> addition to our support for directory externals.
>
> A couple of design questions, but first a design point. I'm implementing
> file externals using the existing support in 1.5 that you can do 'touch foo;
> svn add foo; svn switch foo URL'. So under the hood, having a file external
> sets up a switched file. The existence of some additional fields in the
> entries will tell the client that it still needs to be treated specially.
I haven't completely thought through exactly what you're saying, but
anything that makes "externals" more like "repository-defined
suggestions for switches" is great in my book.
> Because the svn:externals property can add an external to subdirectories and
> users may do updates from a subdirectory, svn may not know that a switched
> path is a file external, so I need to add a marker in the wc entries that
> something is a file external and not just a switched path.
>
> So I can add a single entry line into .svn/entries with a single line,
> either of the two following form:
>
> [HEAD|r\d+] URL
>
> or two separate lines:
>
> URL
> [HEAD|r\d+]
>
> The two have to both be there for this to work, so maybe the first?
Parsing sucks. Later we'll decide that you can support, say, the
{DATE} revision type, which can have a space in it. Be less
ambiguous.
(and re: debuggability, as Karl mentioned: I disbelieve that anybody
is smart enough to consistently figure out what line is what in the
entries file by hand. But do see my svn-entries.el.)
--dave
> Also, representing a HEAD or fixed revision number the svn_opt_revision_t
> seems natural for this, but this has a number of different revision types
> that I don't need. Should I have a new smaller union type that just
> reflects a HEAD revision and a fixed revision?
>
> Any style feedback welcome.
>
> Patch for two separate lines in .svn/entries is below.
>
> Oh yes, and why do we have svn_wc__atts_to_entry()? This looks like it's
> more XML entries files related?
>
> Blair
>
>
>
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 31613)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -1861,6 +1861,23 @@
> * @since New in 1.5. */
> svn_depth_t depth;
>
> + /** The entry is a file external and this is the repository root
> + * relative path to the file, otherwise NULL if the file is not a
> + * file external.
> + *
> + * @since New in 1.6. */
> + const char *file_external_path;
> +
> + /** The entry is a file external and this is the revision number
> + * that the external is checked out from. The only permissable
> + * values are svn_opt_revision_unspecified if the entry is not an
> + * external, svn_opt_revision_head if the external revision is
> + * unspecified or specified with -r HEAD or svn_opt_revision_number
> + * for a specifiec revision number.
> + *
> + * @since New in 1.6. */
> + svn_opt_revision_t file_external_rev;
> +
> /* IMPORTANT: If you extend this structure, check the following functions
> in
> * subversion/libsvn_wc/entries.c, to see if you need to extend them as
> well.
> *
> Index: subversion/libsvn_wc/entries.c
> ===================================================================
> --- subversion/libsvn_wc/entries.c (revision 31613)
> +++ subversion/libsvn_wc/entries.c (working copy)
> @@ -2,7 +2,7 @@
> * entries.c : manipulating the administrative `entries' file.
> *
> * ====================================================================
> - * Copyright (c) 2000-2007 CollabNet. All rights reserved.
> + * Copyright (c) 2000-2008 CollabNet. All rights reserved.
> *
> * This software is licensed as described in the file COPYING, which
> * you should have received as part of this distribution. The terms
> @@ -292,6 +292,41 @@
> return SVN_NO_ERROR;
> }
>
> +/* Parse a file external revision number from a NULL terminated string
> + REV_STR into *RESULT. Valid strings are NULL, "HEAD", or r\d+. A
> + NULL string converts to a HEAD revision. */
> +static svn_error_t *
> +parse_file_external_rev(svn_opt_revision_t *result, const char *rev_str,
> + const char *name)
> +{
> + svn_opt_revision_t r;
> +
> + if (! rev_str)
> + r.kind = svn_opt_revision_head;
> + else
> + {
> + if (strcmp(rev_str, SVN_WC__ENTRY_VALUE_HEAD) == 0)
> + {
> + }
> + else if (rev_str[0] == 'r')
> + {
> + svn_revnum_t rev;
> + SVN_ERR(svn_revnum_parse(&rev, rev_str+1, NULL));
> + r.kind = svn_opt_revision_number;
> + r.value.number = rev;
> + }
> + else
> + return svn_error_createf(SVN_ERR_WC_CORRUPT, NULL,
> + _("Entry for '%s' has invalid file external
> "
> + "revision '%s'"),
> + name ? name : SVN_WC_ENTRY_THIS_DIR,
> + rev_str);
> + }
> + *result = r;
> +
> + return SVN_NO_ERROR;
> +}
> +
> /* Allocate an entry from POOL and read it from [*BUF, END). The
> buffer may be modified in place while parsing. Return the new
> entry in *NEW_ENTRY. Advance *BUF to point at the end of the entry
> @@ -498,6 +533,22 @@
> }
> MAYBE_DONE;
>
> + /* File external relative path. */
> + SVN_ERR(read_str(&entry->file_external_path, buf, end, pool));
> + MAYBE_DONE;
> +
> + /* File external revision. */
> + {
> + const char *rev_str;
> +
> + entry->file_external_rev.kind = svn_opt_revision_head;
> +
> + SVN_ERR(read_str(&rev_str, buf, end, pool));
> + SVN_ERR(parse_file_external_rev(&(entry->file_external_rev), rev_str,
> + name));
> + }
> + MAYBE_DONE;
> +
> done:
> *new_entry = entry;
> return SVN_NO_ERROR;
> @@ -911,6 +962,30 @@
> }
> }
>
> + /* File externals */
> + entry->file_external_path =
> + apr_hash_get(atts, SVN_WC__ENTRY_ATTR_FILE_EXTERNAL_PATH,
> + APR_HASH_KEY_STRING);
> + if (entry->file_external_path)
> + {
> + *modify_flags |= SVN_WC__ENTRY_MODIFY_FILE_EXTERNAL_PATH;
> + entry->file_external_path = apr_pstrdup(pool,
> entry->file_external_path);
> + }
> +
> + {
> + const char *rev_str;
> +
> + rev_str = apr_hash_get(atts, SVN_WC__ENTRY_ATTR_FILE_EXTERNAL_REV,
> + APR_HASH_KEY_STRING);
> + if (rev_str)
> + {
> + SVN_ERR(parse_file_external_rev(&(entry->file_external_rev),
> + rev_str,
> + NULL));
> + *modify_flags |= SVN_WC__ENTRY_MODIFY_FILE_EXTERNAL_REV;
> + }
> + }
> +
> *new_entry = entry;
> return SVN_NO_ERROR;
> }
> @@ -1664,6 +1739,27 @@
> write_val(buf, val, strlen(val));
> }
>
> + /* File externals. */
> + write_str(buf, entry->file_external_path, pool);
> + switch (entry->file_external_rev.kind)
> + {
> + case svn_opt_revision_head:
> + write_val(buf, SVN_WC__ENTRY_VALUE_HEAD,
> + sizeof(SVN_WC__ENTRY_VALUE_HEAD) - 1);
> + break;
> + case svn_opt_revision_number:
> + if (SVN_IS_VALID_REVNUM(entry->file_external_rev.value.number))
> + {
> + svn_stringbuf_appendbytes(buf, "r", 1);
> + write_revnum(buf, entry->file_external_rev.value.number, pool);
> + }
> + else
> + write_val(buf, NULL, 0);
> + break;
> + default:
> + write_val(buf, NULL, 0);
> + }
> +
> /* Remove redundant separators at the end of the entry. */
> while (buf->len > 1 && buf->data[buf->len - 2] == '\n')
> buf->len--;
> @@ -2311,6 +2407,16 @@
> cur_entry->keep_local = FALSE;
> }
>
> + /* File externals. */
> + if (modify_flags & SVN_WC__ENTRY_MODIFY_FILE_EXTERNAL_PATH)
> + cur_entry->file_external_path = (entry->file_external_path
> + ? apr_pstrdup(pool,
> +
> entry->file_external_path)
> + : NULL);
> +
> + if (modify_flags & SVN_WC__ENTRY_MODIFY_FILE_EXTERNAL_REV)
> + cur_entry->file_external_rev = entry->file_external_rev;
> +
> /* Make sure the entry exists in the entries hash. Possibly it
> already did, in which case this could have been skipped, but what
> the heck. */
> @@ -2675,6 +2781,9 @@
> dupentry->cachable_props = apr_pstrdup(pool, entry->cachable_props);
> if (entry->present_props)
> dupentry->present_props = apr_pstrdup(pool, entry->present_props);
> + if (entry->file_external_path)
> + dupentry->file_external_path = apr_pstrdup(pool,
> + entry->file_external_path);
> return dupentry;
> }
>
> Index: subversion/libsvn_wc/wc.h
> ===================================================================
> --- subversion/libsvn_wc/wc.h (revision 31613)
> +++ subversion/libsvn_wc/wc.h (working copy)
> @@ -2,7 +2,7 @@
> * wc.h : shared stuff internal to the svn_wc library.
> *
> * ====================================================================
> - * Copyright (c) 2000-2007 CollabNet. All rights reserved.
> + * Copyright (c) 2000-2008 CollabNet. All rights reserved.
> *
> * This software is licensed as described in the file COPYING, which
> * you should have received as part of this distribution. The terms
> @@ -69,9 +69,11 @@
> * The change from 8 to 9 was the addition of changelists, keep-local,
> * and sticky depth (for selective/sparse checkouts).
> *
> + * The change from 9 to 10 was the addition of file externals.
> + *
> * Please document any further format changes here.
> */
> -#define SVN_WC__VERSION 9
> +#define SVN_WC__VERSION 10
>
> /* A version <= this doesn't have property caching in the entries file. */
> #define SVN_WC__NO_PROPCACHING_VERSION 5
> Index: subversion/libsvn_wc/entries.h
> ===================================================================
> --- subversion/libsvn_wc/entries.h (revision 31613)
> +++ subversion/libsvn_wc/entries.h (working copy)
> @@ -77,12 +77,16 @@
> #define SVN_WC__ENTRY_ATTR_CHANGELIST "changelist"
> #define SVN_WC__ENTRY_ATTR_KEEP_LOCAL "keep-local"
> #define SVN_WC__ENTRY_ATTR_WORKING_SIZE "working-size"
> +#define SVN_WC__ENTRY_ATTR_FILE_EXTERNAL_PATH "file-external-path"
> +#define SVN_WC__ENTRY_ATTR_FILE_EXTERNAL_REV "file-external-rev"
>
> /* Attribute values for 'schedule' */
> #define SVN_WC__ENTRY_VALUE_ADD "add"
> #define SVN_WC__ENTRY_VALUE_DELETE "delete"
> #define SVN_WC__ENTRY_VALUE_REPLACE "replace"
>
> +/* Attribute value for 'file-external' */
> +#define SVN_WC__ENTRY_VALUE_HEAD "head"
>
>
>
> /* Initialize an entries file based on URL at INITIAL_REV, in the adm
> @@ -159,6 +163,8 @@
> #define SVN_WC__ENTRY_MODIFY_CHANGELIST
> APR_INT64_C(0x0000000040000000)
> #define SVN_WC__ENTRY_MODIFY_KEEP_LOCAL
> APR_INT64_C(0x0000000080000000)
> #define SVN_WC__ENTRY_MODIFY_WORKING_SIZE
> APR_INT64_C(0x0000000100000000)
> +#define SVN_WC__ENTRY_MODIFY_FILE_EXTERNAL_PATH
> APR_INT64_C(0x0000000200000000)
> +#define SVN_WC__ENTRY_MODIFY_FILE_EXTERNAL_REV
> APR_INT64_C(0x0000000400000000)
> /* No #define for DEPTH, because it's only meaningful on this-dir anyway.
> */
>
> /* ...ORed together with this to mean "I really mean this, don't be
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: File externals and wc style questions
Posted by Karl Fogel <kf...@red-bean.com>.
Blair Zajac <bl...@orcaware.com> writes:
>>> Oh yes, and why do we have svn_wc__atts_to_entry()? This looks like
>>> it's more XML entries files related?
>>
>> Sure, I assume so. We still have to be able to read those, though,
>> right?
>
> Well, do we? All the newer formats are non-XML, so we would never
> expect to find those newer entry fields in there.
Oh. Heh. Uh, good point, yeah.
>>> +{
>>> + svn_opt_revision_t r;
>>> +
>>> + if (! rev_str)
>>> + r.kind = svn_opt_revision_head;
>>> + else
>>> + {
>>> + if (strcmp(rev_str, SVN_WC__ENTRY_VALUE_HEAD) == 0)
>>> + {
>>> + }
>>
>> ?? :-)
>
> Well, svn_opt_revision_head is the default and if the string is HEAD,
> then don't do anything.
I think it might be better to represent that with a comment. An empty
block just looks like someone got interrupted by the doorbell while
coding or something.
> I was thinking we need to do that. The way this will be implemented
> is that under the hood, file externals will just be switched files.
>
> Say you don't bump the wc format and you have a 1.5 and 1.6 client and
> the 1.6 client makes a file external in a wc. The 1.5 client will see
> it as a simple switched file and possible update it and do other
> operations on it that shouldn't be possible on a file external. So I
> think its safer to bump the wc format.
Mmmmrph. I understand, but you might want to raise this issue in a
separate thread. Working copy bumps can be very painful; we should at
least make sure the dev team agrees that it's worth it here.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: File externals and wc style questions
Posted by Blair Zajac <bl...@orcaware.com>.
Karl Fogel wrote:
> Blair Zajac <bl...@orcaware.com> writes:
>> So I can add a single entry line into .svn/entries with a single line,
>> either of the two following form:
>>
>> [HEAD|r\d+] URL
>>
>> or two separate lines:
>>
>> URL
>> [HEAD|r\d+]
>>
>> The two have to both be there for this to work, so maybe the first?
>
> If they always go together, then yes, I think one line is better.
> (Also, that means there's only one blank line when the field is not
> present, instead of two blank lines -- which doesn't matter much for
> efficiency, but does improve comprehensibilty and debuggability.
>
>> Also, representing a HEAD or fixed revision number the
>> svn_opt_revision_t seems natural for this, but this has a number of
>> different revision types that I don't need. Should I have a new
>> smaller union type that just reflects a HEAD revision and a fixed
>> revision?
>
> Nah, I think just use svn_opt_revision_t and document that certain
> values are not legal in this context.
OK.
>> Oh yes, and why do we have svn_wc__atts_to_entry()? This looks like
>> it's more XML entries files related?
>
> Sure, I assume so. We still have to be able to read those, though,
> right?
Well, do we? All the newer formats are non-XML, so we would never expect to
find those newer entry fields in there.
>
>> Index: subversion/include/svn_wc.h
>> ===================================================================
>> --- subversion/include/svn_wc.h (revision 31613)
>> +++ subversion/include/svn_wc.h (working copy)
>> @@ -1861,6 +1861,23 @@
>> * @since New in 1.5. */
>> svn_depth_t depth;
>>
>> + /** The entry is a file external and this is the repository root
>> + * relative path to the file, otherwise NULL if the file is not a
>> + * file external.
>> + *
>> + * @since New in 1.6. */
>> + const char *file_external_path;
>> +
>> + /** The entry is a file external and this is the revision number
>> + * that the external is checked out from. The only permissable
>> + * values are svn_opt_revision_unspecified if the entry is not an
>> + * external, svn_opt_revision_head if the external revision is
>> + * unspecified or specified with -r HEAD or svn_opt_revision_number
>> + * for a specifiec revision number.
>> + *
>> + * @since New in 1.6. */
>> + svn_opt_revision_t file_external_rev;
>> +
>
> Might be good to document the interdependency between these two
> fields -- that if one is set, the other is set as well.
OK.
>
>> --- subversion/libsvn_wc/entries.c (revision 31613)
>> +++ subversion/libsvn_wc/entries.c (working copy)
>> @@ -292,6 +292,41 @@
>> return SVN_NO_ERROR;
>> }
>>
>> +/* Parse a file external revision number from a NULL terminated string
>> + REV_STR into *RESULT. Valid strings are NULL, "HEAD", or r\d+. A
>> + NULL string converts to a HEAD revision. */
>> +static svn_error_t *
>> +parse_file_external_rev(svn_opt_revision_t *result, const char *rev_str,
>> + const char *name)
>
> Hmm, do we not already have a function somewhere for parsing strings
> into revisions? I mean, the cmdline client has to do this too...
I'm only expecting two types of strings here, r\d+ and HEAD, not PREV, COMMITTED
or anything else. But I can go looking for that function.
> Your doc string doesn't mention the NAME parameter by, uh, name.
>
>> +{
>> + svn_opt_revision_t r;
>> +
>> + if (! rev_str)
>> + r.kind = svn_opt_revision_head;
>> + else
>> + {
>> + if (strcmp(rev_str, SVN_WC__ENTRY_VALUE_HEAD) == 0)
>> + {
>> + }
>
> ?? :-)
Well, svn_opt_revision_head is the default and if the string is HEAD, then don't
do anything.
>
>> + else if (rev_str[0] == 'r')
>> + {
>> + svn_revnum_t rev;
>> + SVN_ERR(svn_revnum_parse(&rev, rev_str+1, NULL));
>> + r.kind = svn_opt_revision_number;
>> + r.value.number = rev;
>> + }
>> + else
>> + return svn_error_createf(SVN_ERR_WC_CORRUPT, NULL,
>> + _("Entry for '%s' has invalid file external "
>> + "revision '%s'"),
>> + name ? name : SVN_WC_ENTRY_THIS_DIR,
>> + rev_str);
>> + }
>> + *result = r;
>> +
>> + return SVN_NO_ERROR;
>> +}
>
> Indentation became inconsistent (two spaces for the top 'else', bt then
> one space for the next level in). Not sure how that happened.
I'll clean up indentation, I just quickly through the patch I had into an email.
>
> (Also, personally I like to use braces on all the bodies if any body has
> them, but I'm not sure how consistent we've been about that. Braces on
> multiline body would be good, too; especially with the odd indentation
> above, it takes a moment to see what's up in that final 'else'.)
>
>> @@ -498,6 +533,22 @@
>> }
>> MAYBE_DONE;
>>
>> + /* File external relative path. */
>> + SVN_ERR(read_str(&entry->file_external_path, buf, end, pool));
>> + MAYBE_DONE;
>> +
>> + /* File external revision. */
>> + {
>> + const char *rev_str;
>> +
>> + entry->file_external_rev.kind = svn_opt_revision_head;
>> +
>> + SVN_ERR(read_str(&rev_str, buf, end, pool));
>> + SVN_ERR(parse_file_external_rev(&(entry->file_external_rev), rev_str,
>> + name));
>> + }
>> + MAYBE_DONE;
>> +
>> done:
>> *new_entry = entry;
>> return SVN_NO_ERROR;
>
> So here's one reason to have them on the same line: you've got a
> MAYBE_DONE in between the two new reads, but actually it would be
> incorrect to be done between them, right?
Well, I'm going to ignore the revision number if the file external path is NULL.
But yes, I'll put both on the same line.
>
>> @@ -911,6 +962,30 @@
>> }
>> }
>>
>> + /* File externals */
>> + entry->file_external_path =
>> + apr_hash_get(atts, SVN_WC__ENTRY_ATTR_FILE_EXTERNAL_PATH,
>> + APR_HASH_KEY_STRING);
>> + if (entry->file_external_path)
>> + {
>> + *modify_flags |= SVN_WC__ENTRY_MODIFY_FILE_EXTERNAL_PATH;
>> + entry->file_external_path = apr_pstrdup(pool, entry->file_external_path);
>> + }
>> +
>> + {
>> + const char *rev_str;
>> +
>> + rev_str = apr_hash_get(atts, SVN_WC__ENTRY_ATTR_FILE_EXTERNAL_REV,
>> + APR_HASH_KEY_STRING);
>> + if (rev_str)
>> + {
>> + SVN_ERR(parse_file_external_rev(&(entry->file_external_rev),
>> + rev_str,
>> + NULL));
>> + *modify_flags |= SVN_WC__ENTRY_MODIFY_FILE_EXTERNAL_REV;
>> + }
>> + }
>> +
>> *new_entry = entry;
>> return SVN_NO_ERROR;
>> }
>
> Why pass NULL for the 'name' param to parse_file_external_rev() here?
> Doesn't svn_wc__atts_to_entry() have the name available?
The name param is just for logging if the revision cannot be parsed. The other
caller to parse_file_external_rev has the name available of the file that had
the error. That may be incorrect, if name is NULL, then it assumes '.', but it
can't always make that assumption.
>
>> --- subversion/libsvn_wc/wc.h (revision 31613)
>> +++ subversion/libsvn_wc/wc.h (working copy)
>> @@ -69,9 +69,11 @@
>> * The change from 8 to 9 was the addition of changelists, keep-local,
>> * and sticky depth (for selective/sparse checkouts).
>> *
>> + * The change from 9 to 10 was the addition of file externals.
>> + *
>> * Please document any further format changes here.
>> */
>> -#define SVN_WC__VERSION 9
>> +#define SVN_WC__VERSION 10
>
> Hmmm. So does this mean we're going to bump people's working copy
> formats (thus making it harder to downgrade back to one's old client
> after trying out a new client) even if they never use file externals?
I was thinking we need to do that. The way this will be implemented is that
under the hood, file externals will just be switched files.
Say you don't bump the wc format and you have a 1.5 and 1.6 client and the 1.6
client makes a file external in a wc. The 1.5 client will see it as a simple
switched file and possible update it and do other operations on it that
shouldn't be possible on a file external. So I think its safer to bump the wc
format.
>
>> --- subversion/libsvn_wc/entries.h (revision 31613)
>> +++ subversion/libsvn_wc/entries.h (working copy)
>> @@ -77,12 +77,16 @@
>> #define SVN_WC__ENTRY_ATTR_CHANGELIST "changelist"
>> #define SVN_WC__ENTRY_ATTR_KEEP_LOCAL "keep-local"
>> #define SVN_WC__ENTRY_ATTR_WORKING_SIZE "working-size"
>> +#define SVN_WC__ENTRY_ATTR_FILE_EXTERNAL_PATH "file-external-path"
>> +#define SVN_WC__ENTRY_ATTR_FILE_EXTERNAL_REV "file-external-rev"
>>
>> /* Attribute values for 'schedule' */
>> #define SVN_WC__ENTRY_VALUE_ADD "add"
>> #define SVN_WC__ENTRY_VALUE_DELETE "delete"
>> #define SVN_WC__ENTRY_VALUE_REPLACE "replace"
>>
>> +/* Attribute value for 'file-external' */
>> +#define SVN_WC__ENTRY_VALUE_HEAD "head"
>
> Recommend "HEAD", since that's how it's commonly written.
>
> Finally: no change to libsvn_wc/README? :-)
I'll add that too.
Blair
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: File externals and wc style questions
Posted by Karl Fogel <kf...@red-bean.com>.
Blair Zajac <bl...@orcaware.com> writes:
> So I can add a single entry line into .svn/entries with a single line,
> either of the two following form:
>
> [HEAD|r\d+] URL
>
> or two separate lines:
>
> URL
> [HEAD|r\d+]
>
> The two have to both be there for this to work, so maybe the first?
If they always go together, then yes, I think one line is better.
(Also, that means there's only one blank line when the field is not
present, instead of two blank lines -- which doesn't matter much for
efficiency, but does improve comprehensibilty and debuggability.
> Also, representing a HEAD or fixed revision number the
> svn_opt_revision_t seems natural for this, but this has a number of
> different revision types that I don't need. Should I have a new
> smaller union type that just reflects a HEAD revision and a fixed
> revision?
Nah, I think just use svn_opt_revision_t and document that certain
values are not legal in this context.
> Any style feedback welcome.
>
> Patch for two separate lines in .svn/entries is below.
Review below.
> Oh yes, and why do we have svn_wc__atts_to_entry()? This looks like
> it's more XML entries files related?
Sure, I assume so. We still have to be able to read those, though,
right?
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 31613)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -1861,6 +1861,23 @@
> * @since New in 1.5. */
> svn_depth_t depth;
>
> + /** The entry is a file external and this is the repository root
> + * relative path to the file, otherwise NULL if the file is not a
> + * file external.
> + *
> + * @since New in 1.6. */
> + const char *file_external_path;
> +
> + /** The entry is a file external and this is the revision number
> + * that the external is checked out from. The only permissable
> + * values are svn_opt_revision_unspecified if the entry is not an
> + * external, svn_opt_revision_head if the external revision is
> + * unspecified or specified with -r HEAD or svn_opt_revision_number
> + * for a specifiec revision number.
> + *
> + * @since New in 1.6. */
> + svn_opt_revision_t file_external_rev;
> +
Might be good to document the interdependency between these two
fields -- that if one is set, the other is set as well.
> --- subversion/libsvn_wc/entries.c (revision 31613)
> +++ subversion/libsvn_wc/entries.c (working copy)
> @@ -292,6 +292,41 @@
> return SVN_NO_ERROR;
> }
>
> +/* Parse a file external revision number from a NULL terminated string
> + REV_STR into *RESULT. Valid strings are NULL, "HEAD", or r\d+. A
> + NULL string converts to a HEAD revision. */
> +static svn_error_t *
> +parse_file_external_rev(svn_opt_revision_t *result, const char *rev_str,
> + const char *name)
Hmm, do we not already have a function somewhere for parsing strings
into revisions? I mean, the cmdline client has to do this too...
Your doc string doesn't mention the NAME parameter by, uh, name.
> +{
> + svn_opt_revision_t r;
> +
> + if (! rev_str)
> + r.kind = svn_opt_revision_head;
> + else
> + {
> + if (strcmp(rev_str, SVN_WC__ENTRY_VALUE_HEAD) == 0)
> + {
> + }
?? :-)
> + else if (rev_str[0] == 'r')
> + {
> + svn_revnum_t rev;
> + SVN_ERR(svn_revnum_parse(&rev, rev_str+1, NULL));
> + r.kind = svn_opt_revision_number;
> + r.value.number = rev;
> + }
> + else
> + return svn_error_createf(SVN_ERR_WC_CORRUPT, NULL,
> + _("Entry for '%s' has invalid file external "
> + "revision '%s'"),
> + name ? name : SVN_WC_ENTRY_THIS_DIR,
> + rev_str);
> + }
> + *result = r;
> +
> + return SVN_NO_ERROR;
> +}
Indentation became inconsistent (two spaces for the top 'else', bt then
one space for the next level in). Not sure how that happened.
(Also, personally I like to use braces on all the bodies if any body has
them, but I'm not sure how consistent we've been about that. Braces on
multiline body would be good, too; especially with the odd indentation
above, it takes a moment to see what's up in that final 'else'.)
> @@ -498,6 +533,22 @@
> }
> MAYBE_DONE;
>
> + /* File external relative path. */
> + SVN_ERR(read_str(&entry->file_external_path, buf, end, pool));
> + MAYBE_DONE;
> +
> + /* File external revision. */
> + {
> + const char *rev_str;
> +
> + entry->file_external_rev.kind = svn_opt_revision_head;
> +
> + SVN_ERR(read_str(&rev_str, buf, end, pool));
> + SVN_ERR(parse_file_external_rev(&(entry->file_external_rev), rev_str,
> + name));
> + }
> + MAYBE_DONE;
> +
> done:
> *new_entry = entry;
> return SVN_NO_ERROR;
So here's one reason to have them on the same line: you've got a
MAYBE_DONE in between the two new reads, but actually it would be
incorrect to be done between them, right?
> @@ -911,6 +962,30 @@
> }
> }
>
> + /* File externals */
> + entry->file_external_path =
> + apr_hash_get(atts, SVN_WC__ENTRY_ATTR_FILE_EXTERNAL_PATH,
> + APR_HASH_KEY_STRING);
> + if (entry->file_external_path)
> + {
> + *modify_flags |= SVN_WC__ENTRY_MODIFY_FILE_EXTERNAL_PATH;
> + entry->file_external_path = apr_pstrdup(pool, entry->file_external_path);
> + }
> +
> + {
> + const char *rev_str;
> +
> + rev_str = apr_hash_get(atts, SVN_WC__ENTRY_ATTR_FILE_EXTERNAL_REV,
> + APR_HASH_KEY_STRING);
> + if (rev_str)
> + {
> + SVN_ERR(parse_file_external_rev(&(entry->file_external_rev),
> + rev_str,
> + NULL));
> + *modify_flags |= SVN_WC__ENTRY_MODIFY_FILE_EXTERNAL_REV;
> + }
> + }
> +
> *new_entry = entry;
> return SVN_NO_ERROR;
> }
Why pass NULL for the 'name' param to parse_file_external_rev() here?
Doesn't svn_wc__atts_to_entry() have the name available?
> @@ -1664,6 +1739,27 @@
> write_val(buf, val, strlen(val));
> }
>
> + /* File externals. */
> + write_str(buf, entry->file_external_path, pool);
> + switch (entry->file_external_rev.kind)
> + {
> + case svn_opt_revision_head:
> + write_val(buf, SVN_WC__ENTRY_VALUE_HEAD,
> + sizeof(SVN_WC__ENTRY_VALUE_HEAD) - 1);
> + break;
> + case svn_opt_revision_number:
> + if (SVN_IS_VALID_REVNUM(entry->file_external_rev.value.number))
> + {
> + svn_stringbuf_appendbytes(buf, "r", 1);
> + write_revnum(buf, entry->file_external_rev.value.number, pool);
> + }
> + else
> + write_val(buf, NULL, 0);
> + break;
> + default:
> + write_val(buf, NULL, 0);
> + }
I guess there's no good way to error on an unexpected case, since
write_entry() returns void. Oh well.
> --- subversion/libsvn_wc/wc.h (revision 31613)
> +++ subversion/libsvn_wc/wc.h (working copy)
> @@ -69,9 +69,11 @@
> * The change from 8 to 9 was the addition of changelists, keep-local,
> * and sticky depth (for selective/sparse checkouts).
> *
> + * The change from 9 to 10 was the addition of file externals.
> + *
> * Please document any further format changes here.
> */
> -#define SVN_WC__VERSION 9
> +#define SVN_WC__VERSION 10
Hmmm. So does this mean we're going to bump people's working copy
formats (thus making it harder to downgrade back to one's old client
after trying out a new client) even if they never use file externals?
> --- subversion/libsvn_wc/entries.h (revision 31613)
> +++ subversion/libsvn_wc/entries.h (working copy)
> @@ -77,12 +77,16 @@
> #define SVN_WC__ENTRY_ATTR_CHANGELIST "changelist"
> #define SVN_WC__ENTRY_ATTR_KEEP_LOCAL "keep-local"
> #define SVN_WC__ENTRY_ATTR_WORKING_SIZE "working-size"
> +#define SVN_WC__ENTRY_ATTR_FILE_EXTERNAL_PATH "file-external-path"
> +#define SVN_WC__ENTRY_ATTR_FILE_EXTERNAL_REV "file-external-rev"
>
> /* Attribute values for 'schedule' */
> #define SVN_WC__ENTRY_VALUE_ADD "add"
> #define SVN_WC__ENTRY_VALUE_DELETE "delete"
> #define SVN_WC__ENTRY_VALUE_REPLACE "replace"
>
> +/* Attribute value for 'file-external' */
> +#define SVN_WC__ENTRY_VALUE_HEAD "head"
Recommend "HEAD", since that's how it's commonly written.
Finally: no change to libsvn_wc/README? :-)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org