More SVN status bugs

[Bill Tutt <bi...@microsoft.com>]

While walking through the various things in svn-test.sh, and getting
WinSVN to display the correct thing in each case, I came across the
following:

* ${SVN_PROG} delete --force ${TEST_DIR_1}\A\D\H\omega

Registers the delete in $(TEST_DIR_1)/SVN/entries with a filename of
"A\D\H\omega" and an ancestor of: "anni/A\D\H\omega"
Doesn't touch ANYTHING in $(TEST_DIR_1)/A/D/H/SVN/entries

* adding a file (newfile1), and then deleting it before you commit it
results in the correct state in the entry structure for it, but there's
an unneeded else that marks this file as just being added:

Index: status.c
===================================================================
RCS file: /cvs/subversion/subversion/libsvn_wc/status.c,v
retrieving revision 1.24
diff -u -r1.24 status.c
--- status.c	2001/01/09 00:05:22	1.24
+++ status.c	2001/01/11 08:52:54
@@ -108,7 +108,7 @@
             status->prop_status = svn_wc_status_added;
         }
 
-      else if (entry->state & SVN_WC_ENTRY_DELETED)
+      if (entry->state & SVN_WC_ENTRY_DELETED)
         {
           status->text_status = svn_wc_status_deleted;
           

* svn_wc_text_modified_p was assuming that just because we could
construct a string variable that might point to the text-base version of
the file, that the actual file exists.

Index: questions.c
===================================================================
RCS file: /cvs/subversion/subversion/libsvn_wc/questions.c,v
retrieving revision 1.44
diff -u -r1.44 questions.c
--- questions.c	2001/01/08 23:06:34	1.44
+++ questions.c	2001/01/11 08:53:21
@@ -349,9 +349,12 @@
   /* Get the full path of the textbase revision of filename */
   textbase_filename = svn_wc__text_base_path (filename, 0, pool);
 
+  err = svn_io_check_path (textbase_filename, &kind, pool);
+  if (err) return err;
+
   /* Simple case:  if there's no text-base revision of the file, all we
      can do is look at timestamps.  */
-  if (! textbase_filename)
+  if (kind != svn_node_file)
     {
       err = timestamps_equal_p (&equal_timestamps, filename,
                                 svn_wc__text_time, pool);

Ok to commit?
Bill

Re: More SVN status bugs

[Karl Fogel <kf...@galois.collab.net>]

"Bill Tutt" <bi...@microsoft.com> writes:
> * ${SVN_PROG} delete --force ${TEST_DIR_1}\A\D\H\omega
> 
> Registers the delete in $(TEST_DIR_1)/SVN/entries with a filename of
> "A\D\H\omega" and an ancestor of: "anni/A\D\H\omega"
> Doesn't touch ANYTHING in $(TEST_DIR_1)/A/D/H/SVN/entries

Okay; working on, thanks for noticing this!  (Time to get that path
library fleshed out. :-) ).

> * adding a file (newfile1), and then deleting it before you commit it
> results in the correct state in the entry structure for it, but there's
> an unneeded else that marks this file as just being added:
> 
> Index: status.c
> ===================================================================
> RCS file: /cvs/subversion/subversion/libsvn_wc/status.c,v
> retrieving revision 1.24
> diff -u -r1.24 status.c
> --- status.c	2001/01/09 00:05:22	1.24
> +++ status.c	2001/01/11 08:52:54
> @@ -108,7 +108,7 @@
>              status->prop_status = svn_wc_status_added;
>          }
>  
> -      else if (entry->state & SVN_WC_ENTRY_DELETED)
> +      if (entry->state & SVN_WC_ENTRY_DELETED)
>          {
>            status->text_status = svn_wc_status_deleted;

Hmm.  If we get rid of the `else', then the result of the second
conditional body might simply override the result of the first.  In
other words, something that was marked as *both* added and deleted
would only show up as deleted.  Which would be fairly random -- if the
conditionals' order were reversed, then it would be adds overriding
deletes instead of the other way around.

I think the `else' is deliberate.  Here's the reasoning:

   1. If an entry is just added, you want to report it as added.

   2. If an entry is just deleted, you want to report it as deleted.

   3. If an entry is _both_ added and deleted, that can only mean one
      thing: the entry was deleted and *then* added.  That is, in the
      working copy, someone removed a file, then they added a new file
      under the same name.  (Had the order been reversed, then the
      entry would simply have been removed -- there's no point even
      talking about a file that was added and then un-added.  It never
      happened.)  So the idea is that status reports it as added.

The current conditional setup supports this logic.  (But maybe there
should be a comment to this effect nearby!)

Another solution is to have a special status for "replaced", that is,
deleted and then added.  Thoughts?

> * svn_wc_text_modified_p was assuming that just because we could
> construct a string variable that might point to the text-base version of
> the file, that the actual file exists.

Nice catch. :-) Yeah, your patch for this looks like exactly the right
thing, go ahead and commit.

Note that that whole block of code (handling the case where the
text-base file doesn't exist) was written in anticipation of the day
when working copies don't require a text base.  That day is not today,
nor is it the near future.  However, when and if that day arrives,
this function will be prepared for it. :-)

-K


> Index: questions.c
> ===================================================================
> RCS file: /cvs/subversion/subversion/libsvn_wc/questions.c,v
> retrieving revision 1.44
> diff -u -r1.44 questions.c
> --- questions.c	2001/01/08 23:06:34	1.44
> +++ questions.c	2001/01/11 08:53:21
> @@ -349,9 +349,12 @@
>    /* Get the full path of the textbase revision of filename */
>    textbase_filename = svn_wc__text_base_path (filename, 0, pool);
>  
> +  err = svn_io_check_path (textbase_filename, &kind, pool);
> +  if (err) return err;
> +
>    /* Simple case:  if there's no text-base revision of the file, all we
>       can do is look at timestamps.  */
> -  if (! textbase_filename)
> +  if (kind != svn_node_file)
>      {
>        err = timestamps_equal_p (&equal_timestamps, filename,
>                                  svn_wc__text_time, pool);