On Thu, Apr 22, 2010 at 02:45:08PM -0400, Greg Stein wrote:
> On Thu, Apr 22, 2010 at 10:05, Daniel Näslund <daniel_at_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
Received on 2010-04-22 21:50:10 CEST