On Thu, Apr 22, 2010 at 10:05, Daniel Näslund <daniel_at_longitudo.com> wrote:
> Hi!
>
> [[[
> As part of WC-NG, replace entry calls for revisions from
> svn_wc_status2_t related code.
>
> The entries code set the revision of newly added nodes to 0 but the db
> sets them to -1. Since too many tests needs to be changed and 'svn info'
> also uses 0, I'll change those values in a follow-up patch instead.
>
> * subversion/tests/cmdline/copy_tests.py
> (repos_to_wc): Change revision to 0 since the node is added. Don't
> check against entries since the behavior differs.
>
> * subversion/tests/cmdline/stat_tests.py
> (status_with_tree_conflicts): An copied node should not have a
> changed_rev or author.
Don't the changed_rev and changed_author correspond to the repository
node that was copied into the tree? Thus, when I'm looking at a diff,
it is effectively relative to what I copied in.
> * subversion/tests/cmdline/svntest/actions.py
> (run_and_verify_status): Add new parameter check_entries. We only
> check the entries if check_entries is set to True.
No way. For the *one* item which needs a different revision... maybe
okay. But by eliminating the check_entries, you're also eliminating
the checks on all the *other* items in the status output.
Instead, I recommend changing svntest/wc.StateItem. Add an "entry_rev"
attribute defaulting to None. Where we expect the entry's revision to
differ from that reported in a typical status output, then we can set
this extra attribute and specifically track the change in behavior.
And that's what we're talking about here: a change in behavior from
1.6 to 1.7. The entries testing is in there to ensure that we don't
(arbitrarily) change the revision information that we display.
>
> * subversion/tests/cmdline/merge_tests.py
> (merge_into_missing): Add revision to status output for missing dir.
> Previously, missing dirs did not have a revision, only files had.
> Don't check entries since the behavior differs.
>
> * subversion/svn/status.c
> (print_status): Replace checks for revisions using entries with the
> direct fields in svn_wc_status3_t. Set the revision for added and
> replaced nodes to 0. WC-NG sets those revisions to -1, but changing
> all the involved tests is for a follow-up.
>
> * subversion/include/svn_wc.h
> (svn_wc_status2_t): Add fields revision, changed_rev and
> changed_author.
If you have changed_rev and changed_author, then please include
changed_date. It is weird/inconsistent to have two of the three.
Also: your log message says svn_wc_status2_t, but you changed 3.
And now that you've changed status3, you should alter the
svn_wc__status2_from_3() function.
And you don't need @since tags in this part of the change, since the
whole structure is new.
> * subversion/libsvn_wc/status.c
> (assemble_status): Fill in the new fields with data from the wc db.
> (svn_wc_dup_status3): Copy the changed_author field.
> ]]]
>
> Quirks
> ========
>
> Checking entries
> -------------------
> I need to tweak the status output when doing a compare against the
> entries in run_and_verify_status(). The code changes the behaviour for
> revisions in three ways:
>
> i) Missing dirs, now have a revision
This seems fine.
> ii) Copies from foreign repos to wc has the rev set to -1
Hmm? "foreign repos" only makes sense for merges, I believe. I think
you mean "copy from (our) repository to wc".
The original status code would show copyfrom_rev here, right? IOW, yes
the status3_t struct should show revision=SVN_INVALID_REVNUM, but I
think the status cmdline would show copyfrom_rev.
> iii) A copied node should not have a changed_author or changed_rev set.
I disagree, per above.
read_info() returns changed_* information for copied nodes. A copied
node has a corresponding node in the repository, and the changed_*
information reflects that. (thus, your assemble_status should not be
wiping the values out... if the caller wants to interpret added nodes
that way, then it can)
> I could change tweak_for_entries_compare() to fix i) but for the
> other two I would have to write a func for tweaking the status returned
> by 'entries-dump'. Can't I just use the check_entries parameter I've
> added to run_and_verify_status() instead? I would only disable the
> entries check for three tests.
For the whole *test*. Not just the different items.
Instead, alter tweak_for_entries_compare() to do something like:
if item.entry_rev is not None:
item.wc_rev = item.entry_rev
> Getting revisions of deleted nodes
> -----------------------------------
> I'm using base_get_info() for both nodes having the op_root from a plain
> delete and those from a delete within an add. In the later case, the
> revision, changed_rev and changed_author will be set to the default nil
> values which match what I would get from read_info().
Whoop. Careful here. Deleted nodes may not have an underlying BASE
node that you can fetch a revision from. Consider:
$ svn cp A B
$ svn rm B/foo
WORK_DEL_ABSPATH will be B/foo, and there it has no BASE node.
The question is: do you want to use the copyfrom_rev in this case? Or
do you want to use B's parent's revision?
Personally, I would suggest that assemble_status() does NOTHING more
than returning read_info's REVISION result. That is the "ideal" value
for the revision of the node LOCAL_ABSPATH.
If that revision is NOT what you want (ie. you have some other rules
like "well... if it is added, then ..." or "if it is deleted, then
..."), then the caller should perform those calculations explicitly.
The status code should not pre-suppose what rules the caller may want
for revision handling. It should simply report the "ideal" revision
for that node and be done with it.
Cheers,
-g
Received on 2010-04-22 20:45:36 CEST