On Thu, Apr 22, 2010 at 02:45:08PM -0400, Greg Stein wrote: > On Thu, Apr 22, 2010 at 10:05, Daniel Näslund <dan...@longitudo.com> wrote: > > [[[ > > 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.
I had my doubts about this one, although I didn't express them here. Since a copied or moved node has a previous line of history it makes sense to use that history for changed_rev and changed_author. My motivation for not setting those for copied/moved nodes was that I thought that changed_rev was defined as the closest rev less than or equal to the current revision. Since changed_author is closely related to changed_rev I set that one to it's nil value too. > > > * 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. Ok, I of course prefer this suggestion since it offers the same test coverage but allows me to change the behaviour. But I would have _never_ come up with it on my own. > > 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. I understand and totally agree on that we should be able to easily see in what way our new code differs from the entries-based. > > > > * 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. Ok, the reason it was left out was due to me looking in svn/status.c to determine what fields were used. I didn't see it so it was not included. I took the idea of the status code just returning the most cruciual parts too literally. What I should have done was look at what performance impact there was to including different fields. If I include the others, there won't be much overhead (virtually zero) to including the changed_date as well. Will do. > > Also: your log message says svn_wc_status2_t, but you changed 3. Oups. I thought I changed it but, well, no I didn't. :( > And now that you've changed status3, you should alter the > svn_wc__status2_from_3() function. Since the old code uses the fields from the entry and the entry is still there, nothing needs to be changed in status2_from_3(), *yet*. > 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". Nope, check out copy_tests.py:978. > > 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) Ok, for the reasons I stated earlier. > > 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? Yeah, I read the get_base_info() code as if it would return nil values if there was no BASE and that all cases where there was no BASE node would automatically mean; 'use the nil values'. I agree that it was a dumb idea. > > 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. WHAT? You're saying that all the scanning should take place in the client callback? That's a major change from what I've done so far. That I should just return SVN_INVALID_REVNUM for cases where I've scanned for deletion? Is that a behaviour that everyone agrees on? You're saying personally so I'm assuming there hasn't been a discussion about it. Cheers, Daniel
