I'm running 'make check' on this now, and will commit when done. See
below for review...
Vivek <vivek@collab.net> writes:
> [[[
> Fix svn cat to skip unversioned items
>
> * subversion/clients/cmdline/cat-cmd.c
> (svn_cl__cat): Added logic to skip unversioned items.
>
> * subversion/tests/clients/cmdline/cat_tests.py
> (cat_skip_unversioned_file): New test.
> (test_list): Run it.
> ]]]
This is part of issue #2030, so the log message should say that.
Also, the operative distinction here is cattable/uncattable, not
versioned/unversioned, because versioned directories are not cattable
either. I changed the log message and the test name to reflect this.
(Also, don't forget the period at the end of the first sentence.)
> Index: subversion/clients/cmdline/cat-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/cat-cmd.c (revision 14922)
> +++ subversion/clients/cmdline/cat-cmd.c (working copy)
> @@ -57,6 +57,7 @@
> const char *target = ((const char **) (targets->elts))[i];
> const char *truepath;
> svn_opt_revision_t peg_revision;
> + svn_boolean_t success;
>
> svn_pool_clear (subpool);
> SVN_ERR (svn_cl__check_cancel (ctx->cancel_baton));
> @@ -65,9 +66,14 @@
> SVN_ERR (svn_opt_parse_path (&peg_revision, &truepath, target,
> subpool));
>
> - SVN_ERR (svn_client_cat2 (out, truepath, &peg_revision,
> - &(opt_state->start_revision),
> - ctx, subpool));
> + SVN_ERR (svn_cl__try( svn_client_cat2 (out, truepath, &peg_revision,
> + &(opt_state->start_revision),
> + ctx, subpool),
> + &success, opt_state->quiet,
> + SVN_ERR_UNVERSIONED_RESOURCE,
> + SVN_ERR_ENTRY_NOT_FOUND,
> + SVN_ERR_CLIENT_IS_DIRECTORY,
> + SVN_NO_ERROR));
> }
> svn_pool_destroy (subpool);
Instead of using an ignored 'success' var, we can just pass NULL for
the success parameter to svn_cl__try(). I realize that svn_cl__try()
acquired that ability in r14989, after you submitted the above patch,
so it's understandable that you didn't take advantage of it! :-)
> Index: subversion/tests/clients/cmdline/cat_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/cat_tests.py (revision 14922)
> +++ subversion/tests/clients/cmdline/cat_tests.py (working copy)
> @@ -84,6 +84,56 @@
> None, svntest.SVNAnyOutput, 'cat',
> bogus_path)
>
> +def cat_skip_unversioned_file(sbox):
> + "cat should skip an unversioned resource"
> + sbox.build()
Typically we mention in a comment which issue # the test addresses.
I changed "unversioned" to "uncattable", as discussed earlier. And
it's not just testing files, but directories too, so I took "_file"
out of the test's name.
> + wc_dir = sbox.wc_dir
> + dir_path = os.path.join(wc_dir, 'A/D')
> + new_file_path = os.path.join(dir_path, 'new')
> + open(new_file_path, 'w')
> + item_list = os.listdir(dir_path)
There's trailing whitespace after one of the lines above. This isn't
normally a big deal, but it may result from the same cause as a more
serious trailing-whitespace problem discussed a bit farther down.
> + # item_list has all the files and directories under 'dir_path'
> + # including .svn. But cat'ing of .svn just gives the
> + # output of 'svn help cat'. So skip .svn directory and continue.
This comment describes a minor modifying detail of your algorithm,
without ever describing the algorithm itself. In general, everyone
knows why '.svn' is special, so you don't need to say anything about
it -- that code is self-explanatory. Instead, if you're going to say
anything, say how the test works overall: that you try cat'ing
individual files, some of which are versioned and some of which are
not, and then later you try cat'ing a mixture of cattable and
uncattable objects in the same command.
> + for file in item_list:
> + if file == '.svn': continue
I like to put the 'continue' on its own line so that its position as
the consequent of a conditional is visually apparent. However, this
is a matter of taste.
> + item_to_cat = os.path.join(dir_path, file)
> + if item_to_cat == new_file_path:
> + expected_err = "svn: warning: '" + item_to_cat + "'" + \
> + " is not under version control or doesn't exist\n"
> + svntest.actions.run_and_verify_svn(None, None,
> + expected_err, 'cat', item_to_cat)
The whitespace at the end of the last line above extends to way past
80 columns. I suggest setting your editor to 80 columns when you work
on SVN code, so that you can never fail to spot overruns.
More importantly, your Python indentation level appears to be 4,
whereas the rest of this file (and the other .py files in this
directory) uses 2. Consistent indentation is important even just for
appearances, but it's *especially* important in Python :-). The code
would run okay, I think, because your indentation was consistent
within a top-level form (the function), but if someone else started
editing it, all sorts of confusion might occur.
> + elif os.path.isdir(item_to_cat):
> + expected_err = "svn: warning: '" + item_to_cat + "'" + \
> + " refers to a directory\n"
> + svntest.actions.run_and_verify_svn(None, None,
> + expected_err, 'cat', item_to_cat)
> + else:
> + svntest.actions.run_and_verify_svn(None,
> + ["This is the file '"+file+"'."],
> + None, 'cat', item_to_cat)
> +
> + G_path = os.path.join(dir_path, 'G')
> + rho_path = os.path.join(G_path, 'rho')
> +
> + expected_out = ["This is the file 'rho'."]
> + expected_err1 = ["svn: warning: '" + G_path + "'" + " refers to a directory\n"]
This line extends beyond 80 columns, and it would even without the
trailing whitespace. Various other lines above also have trailing
whitespace, although not to the point of exceeding 80 columns.
Really 79 or 78 is the limit, but if your editor is set to 80, that'll
be easy to spot.
> + svntest.actions.run_and_verify_svn(None, expected_out, expected_err1,
> + 'cat', rho_path, G_path)
> +
> + expected_err2 = ["svn: warning: '" + new_file_path + "'" + \
> + " is not under version control or doesn't exist\n"]
You don't need a backslash here, it's already in square braces.
> + svntest.actions.run_and_verify_svn(None, expected_out, expected_err2,
> + 'cat', rho_path, new_file_path)
> +
> + svntest.actions.run_and_verify_svn(None, expected_out, expected_err1 + \
> + expected_err2, 'cat', rho_path, G_path,
> + new_file_path)
Here you shouldn't use the backslash like that, because it would be
better to keep the two concatenated strings on the same line anyway.
In other words, line break before "expected_err1", and then again
before "'cat'",
Also, the same trailing-whitespace-to-beyond-80-columns thing.
> +
> +
> ########################################################################
> # Run the tests
>
> @@ -93,7 +143,8 @@
> cat_local_directory,
> cat_remote_directory,
> cat_base,
> - cat_nonexistant_file
> + cat_nonexistant_file,
> + cat_skip_unversioned_file,
> ]
Thanks for adding the trailing comma, so the next person doesn't have
to make a two-line change, that's good.
(Hmm, "nonexistent" is misspelled in the old test name, but that's not
a problem with this patch.)
No need to fix any of the stuff discussed above; I fixed it when I
committed, I just wanted to let you know about it.
One thing I did not take care of was this, though:
I think there's a minor conceptual problem with this test. This issue
is really about what happens when someone invokes 'svn cat' on
multiple targets, in which the cattable targets *follow* uncattable
ones. For example:
$ svn cat versioned_file_1 unversioned_file_1 versioned_file_2
[contents of versioned_file_1]
[error for unversioned_file_1]
[contents of versioned_file_2 never seen, due to above error]
$
After your bugfix, of course, it should be:
$ svn cat versioned_file_1 unversioned_file_1 versioned_file_2
[contents of versioned_file_1]
[error for unversioned_file_1]
[contents of versioned_file_2]
$
So ideally, you should test this scenario. However, your final test
group doesn't do that. It does something similar, but not the same:
G_path = os.path.join(dir_path, 'G')
rho_path = os.path.join(G_path, 'rho')
expected_out = ["This is the file 'rho'."]
expected_err1 = ["svn: warning: '" + G_path + "'"
+ " refers to a directory\n"]
svntest.actions.run_and_verify_svn(None, expected_out, expected_err1,
'cat', rho_path, G_path)
expected_err2 = ["svn: warning: '" + new_file_path + "'"
+ " is not under version control or doesn't exist\n"]
svntest.actions.run_and_verify_svn(None, expected_out, expected_err2,
'cat', rho_path, new_file_path)
svntest.actions.run_and_verify_svn(None, expected_out,
expected_err1 + expected_err2,
'cat', rho_path, G_path, new_file_path)
In each case, the only cattable path ('rho_path') *precedes* the
uncattable paths, so of course rho_path will be catted whether or not
there were skipped targets.
In the final test, you do make sure you get warnings for both G_path
and new_file_path, so at least we know that svn tried to do something
after encountering the first unversioned path. That's a good thing to
test. But why not also test the main use case: that a cat can
*succeed* on one target *after* failing on another?
After this is committed, could you patch the test to do that too?
Thanks,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jun 16 17:56:44 2005