[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Issue #1553: revision numbers of added/replaced items

From: <kfogel_at_collab.net>
Date: 2005-04-02 00:09:26 CEST

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

This is an archived mail posted to the Subversion Dev mailing list.