Blair Zajac <blair_at_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_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-06-06 19:07:38 CEST