I'd like to discuss some of the questions raised by Issue #1553.
If you try to read over the history of the issue, you'll probably get
dizzy. Let me try to summarize what's happened so far:
1. We noticed that items scheduled for addition in the working copy
got a revision number. Under some circumstances, that revision
number would be inherited from the parent directory, under
others it might be 0. It was all very complex, I won't go into
the details here.
"Well," we said, "That's silly, a schedule-add file should be at
svn_invalid_revnum, and in 'status -v' output it should show up
with a working revision of '-'."
(The issue actually talks first about copies -- adds with
history -- but later it encompassed all adds.)
2. We tried to implement this invalid-revnum thing, and discovered
that our test suite was making all sorts of confusing and wrong
assumptions. The whole thing blew up into a big fix, eventually
including the changes associated with issue #2257. But I'm over
that now; I'm not bitter.
3. The latest iteration of the patch in issue #1553 has a rather
small C code change, plus broad changes across the test suite
to make it deal with the new status output. However, there are
still two tests failing.
Okay, that brings us to the present. The two tests still failing are:
diff_tests.py 4: diff_replace_a_file()
update_tests.py 3: update_ignores_added()
Although they are failing for "the same" reason, in a sense, the
failures are very different. I'll address each one separately.
The 'diff_replace_a_file' failure is actually because the #1553 C-code
changes *partially fix* a bug in our diff output. The bug is
described in detail in new issue #2261. Unfortunately, the test is
currently written to expect the bogus output shown in #2261. In other
words, the test currently succeeds when it really shouldn't.
Solution: we should decide how we want the diff output to look, change
the test to expect that (this may be complex, due to the highly
abstracted way diff_tests.py is written), and run it XFail until the
bug is fully fixed. Fixing the diff-a-replaced-file bug is rather
beyond the scope of #1553; it's an accident that the #1553 patch made
it change in the right direction at all, really.
The 'update_ignores_added' failure is a manifestation of a deeper
question: what revision number should status report for a
schedule-replace file? With current Subversion, here's what happens:
$ svn status -vuq README
13832 12812 kfogel README
Status against revision: 13844
$ svn rm README
D README
$ echo "totally new file" > README
$ svn add README
A README
$ svn st -vuq README
R 13832 12812 kfogel README
Status against revision: 13844
$
With the latest #1553 patch applied, it does this instead:
$ svn status -vuq README
13832 12812 kfogel README
Status against revision: 13844
$ svn rm README
D README
$ echo "totally new file" > README
$ svn add README
A README
$ svn st -vuq README
R - 12812 kfogel README
Status against revision: 13844
$
It's not clear to me what the right answer is. One the one hand, the
user asked for the status of 'README' as it currently stands, which is
a new schedule-add file, so "-" for a working revision seems right.
On the other hand, it's not like the user had any *other* way to refer
to the old, now-shadowed 'README'. And what's with the 12812?
I feel I may be too close to this problem now, and not thinking
clearly about it. Ben Collins-Sussman has proposed that the
conditionals in the #1553 patch (see the entire C-code patch below)
should only ask about schedule_add, not about schedule_replace.
Thoughts?
Here's the C change; issue #1553 has the log msg and test changes too.
Index: subversion/libsvn_wc/entries.c
===================================================================
--- subversion/libsvn_wc/entries.c (revision 13832)
+++ subversion/libsvn_wc/entries.c (working copy)
@@ -567,8 +567,11 @@
take_from_entry (svn_wc_entry_t *src, svn_wc_entry_t *dst, apr_pool_t *pool)
{
/* Inherits parent's revision if doesn't have a revision of one's
- own, unless this is a subdirectory. */
- if ((dst->revision == SVN_INVALID_REVNUM) && (dst->kind != svn_node_dir))
+ own, unless this is a subdirectory or if it is schedule
+ add/replace. */
+ if ((dst->revision == SVN_INVALID_REVNUM) && (dst->kind != svn_node_dir) &&
+ ! ((dst->schedule == svn_wc_schedule_add) ||
+ (dst->schedule == svn_wc_schedule_replace)))
dst->revision = src->revision;
/* Inherits parent's url if doesn't have a url of one's own. */
@@ -600,7 +603,9 @@
NULL,
_("Missing default entry"));
- if (default_entry->revision == SVN_INVALID_REVNUM)
+ if ((default_entry->revision == SVN_INVALID_REVNUM) &&
+ ! ((default_entry->schedule == svn_wc_schedule_add)
+ || (default_entry->schedule == svn_wc_schedule_replace)))
return svn_error_create (SVN_ERR_ENTRY_MISSING_REVISION,
NULL,
_("Default entry has no revision number"));
Index: subversion/libsvn_wc/adm_ops.c
===================================================================
--- subversion/libsvn_wc/adm_ops.c (revision 13832)
+++ subversion/libsvn_wc/adm_ops.c (working copy)
@@ -1006,7 +1006,7 @@
modify_flags |= SVN_WC__ENTRY_MODIFY_COPIED;
}
- tmp_entry.revision = 0;
+ tmp_entry.revision = SVN_INVALID_REVNUM;
tmp_entry.kind = kind;
tmp_entry.schedule = svn_wc_schedule_add;
Index: subversion/clients/cmdline/status.c
===================================================================
--- subversion/clients/cmdline/status.c (revision 13832)
+++ subversion/clients/cmdline/status.c (working copy)
@@ -68,9 +68,8 @@
if (! status->entry)
working_rev = "";
- else if (! SVN_IS_VALID_REVNUM (status->entry->revision))
- working_rev = " ? ";
- else if (status->copied)
+ else if ((! SVN_IS_VALID_REVNUM (status->entry->revision))
+ || status->copied)
working_rev = "-";
else
working_rev = apr_psprintf (pool, "%ld", status->entry->revision);
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Apr 2 00:35:19 2005