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
Received on Sat May 21 00:09:49 2005