Re: [PATCH]: status "I" not consistently shown for ignored directory

Julian Foad <julianfoad@btopenworld.com> writes:

> The fix duplicates a block of code from further up in the same
> function (which handled a file rather than a directory). I could
> not see an elegant way to avoid that, but would like it if someone
> else can.

Make it a separate function that gets called from the two places?

> The test that I added is a copy-paste-modify of the existing
> "status_for_unignored_file" test. I tried combining them into a
> single test but could not do this elegantly without rewriting the
> existing test significantly. I feared that rewriting an existing
> test is a bad idea because of the risk of breaking it. Is there a
> policy or a feeling on this?

Changing the existing test should be OK.

> Also I added the new test immediately after the test that it is
> similar to, thus renumbering the tests that come after it. Should I
> have added it at the end instead to preserve the numbering?

It's fine where it is, although if you modify an existing test then
question goes away.

> Index: subversion/tests/clients/cmdline/stat_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/stat_tests.py (revision 6567)
> +++ subversion/tests/clients/cmdline/stat_tests.py (working copy)
> @@ -361,6 +361,55 @@
> os.chdir(was_cwd)
>
>
> +# Regression: in r6550, "svn status unversioned-ignored-dir" was reporting
> +# "? unversioned-ignored-dir"
> +# while "svn status --no-ignore" was correctly reporting
> +# "I unversioned-ignored-dir"
> +
> +def status_for_unignored_dir(sbox):
> + "status for unignored dir"
> +
> + sbox.build()
> +
> + wc_dir = sbox.wc_dir
> + was_cwd = os.getcwd()
> +
> + os.chdir(wc_dir)
> +
> + try:
> + os.makedirs('newdir')
> + svntest.main.run_svn(None, 'propset', 'svn:ignore', 'newdir', '.')
> +
> + # status on the parent directory with --no-ignore
> + stat_output, err_output = svntest.main.run_svn(None, 'status',
> + '--no-ignore', '.')
> + if err_output:
> + raise svntest.Failure
> + status = 1
> + for line in stat_output:
> + if re.match("I +newdir", line):
> + status = 0
> +
> + if (status == 1):
> + raise svntest.Failure

An RE is probably over the top as the number of spaces should be
known. When I wrote something like that someone (Greg?) suggested

   for line in stat_output:
     if line.find("I newdir") != -1:
       break
   else:
     raise svntest.Failure

However I would probably drop run_svn altogether. It would be better
to use run_and_verify_status but I don't know if that supports 'I', if
it doesn't I would use something like

   run_and_verify_svn(None, ['I newdir\n'], [], 'status', '--no-ignore')

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