Karl Fogel wrote:
> 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.
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_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-06-09 17:22:32 CEST