[PATCH] fix svn cat to skip unversioned items V2.

[vi...@collab.net]

Fix svn cat so that it skips unversioned items

* subversion/clients/cmdline/cat-cmd.c
  (svn_cl__cat): Added logic to skip unversioned items.
  Also skips directories.

* subversion/tests/clients/cmdline/cat_tests.py
  (cat_skip_unversioned_file): New function to
  verify skipping of unversioned items.

Re: [PATCH] fix svn cat to skip unversioned items V2.

[Daniel Rall <dl...@finemaltcoding.com>]

On Fri, 2005-05-20 at 07:56 -0700, vivek@collab.net wrote:
>Fix svn cat so that it skips unversioned items
>
>* subversion/clients/cmdline/cat-cmd.c
>  (svn_cl__cat): Added logic to skip unversioned items.
>  Also skips directories.
>
>* subversion/tests/clients/cmdline/cat_tests.py
>  (cat_skip_unversioned_file): New function to
>  verify skipping of unversioned items.
...

I've tested this patch and found it to work well, but am curious whether
this behavior is 1) consistent with other command-line client behavior,
and if not, 2) the type of experience that users will actually want.

Considering the case that a user makes a typo on the command-line while
casually perusing the repository, the behavior seems great.  But
considering the case where the user is either gathering the contents of
a few specific files to exchange will a business partner, or scripting
some operations using the command-line client, should any of the
requested files be cat'ed when some of them cannot be?

Vivek, a few misc. comments inline on the patch and test case:

>--- subversion/clients/cmdline/cat-cmd.c        (revision 14784)
>+++ subversion/clients/cmdline/cat-cmd.c        (working copy)
...
>@@ -65,9 +66,22 @@
>       SVN_ERR (svn_opt_parse_path (&peg_revision, &truepath, target,
>                                    subpool));
>       
>-      SVN_ERR (svn_client_cat2 (out, truepath, &peg_revision,
>+      err = svn_client_cat2 (out, truepath, &peg_revision,
>                                 &(opt_state->start_revision),
>-                                ctx, subpool));
>+                                ctx, subpool);

The parameters in this list should be horizontally aligned, shifting the
following lines three characters to the left.

...
>--- subversion/tests/clients/cmdline/cat_tests.py       (revision
>14784)
>+++ subversion/tests/clients/cmdline/cat_tests.py       (working copy)
>@@ -84,6 +84,44 @@
>                                      None, svntest.SVNAnyOutput,
>'cat',
>                                      bogus_path)
> 
>+# I couldn't think of a way of testing cat with multiple arguments,
>+# some of which would be files and others directories. But effectively
>it 
>+# can be simulated in a loop, or so I
>think. :-)                                 

Testing with multiple arguments is an important use case for this change
in behavior, as it will be a common situation that a user typos one
target in their WC, but not another.

>+def cat_skip_unversioned_file(sbox):
>+  "cat should skip an unversioned file"
>+  sbox.build()
>+    
>+  wc_dir = sbox.wc_dir
>+  dir_path = os.path.join(wc_dir, 'A/D')
>+  new_file_path = os.path.join(dir_path, 'new')
>+  new_file = open(new_file_path, 'w')

You really needn't retain a reference to new_file.  It will be
automatically close()'d by Python with the ref count hits zero.

>+    

Extraneous whitespace.

>+  file_list = os.listdir(dir_path)
>+  #There is a small problem. File_list gives all the files inside

You can remove the "There is a small problem." sentence, and just state
the issue.  file_list needn't be capitalized -- easier to understand
that way.  Also, how about a space between the comment character # and
your comment's text?  Again, a little easier on the eyes that way.

>+  #the directory including .svn. But cat'ng of .svn just gives the 

How about "cat'ing" instead of "cat'ng"?

>+  #output of 'svn help cat'. So skip the directory .svn
>+  file_list = file_list[1:]
>+    
>+  for file in file_list :
>+     file_to_cat = os.path.join(dir_path, file)
>+     if(file_to_cat == new_file_path):
         ^
No need for this paren (nor the others like it).  Other code in this
file is a bad example -- that's not a Pythonic idiom.

>+        expected_err = "svn: warning: '" + file_to_cat + "'" + \
>+            " is not under version control or doesn't exist\n"
>+        svntest.actions.run_and_verify_svn(None, None, 
>+                                      expected_err, 'cat',
>file_to_cat)                        

Indentation doesn't match up, extraneous whitespace after closing paren.

>+     elif(os.path.isdir(file_to_cat)):
>+           expected_err = "svn: warning: '" + file_to_cat + "'" + \
>+              " refers to a directory\n" 
>+           svntest.actions.run_and_verify_svn(None, None, 
>+                                               expected_err, 'cat',
>file_to_cat)  
>+     else:
>+           svntest.actions.run_and_verify_svn(None, 
>+           ["This is the file '"+file+"'."], None, 'cat', file_to_cat)
>+           

Similar issues with this section as mentioned above.

>+  new_file.close()
>+  os.remove(new_file_path)
...
>@@ -93,7 +131,8 @@
>               cat_local_directory,
>               cat_remote_directory,
>               cat_base,
>-              cat_nonexistant_file
>+              cat_nonexistant_file,
>+              cat_skip_unversioned_file
...

Include a comma on the last parameter, so that the next addition of a
test function to the list doesn't create a spurious diff from adding a
comment to the function which was previously last (like the one you see
above).



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org