You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2003/07/25 16:53:43 UTC
[PATCH]: status "I" not consistently shown for ignored directory
This patch fixes the following anomaly:
~/src/subversion/svn-status-tests> mkdir ignored-dir
~/src/subversion/svn-status-tests> touch ignored-file
~/src/subversion/svn-status-tests> svn propset svn:ignore 'ignored-*' .
property `svn:ignore' set on ''
~/src/subversion/svn-status-tests> svn status --no-ignore
A .
I ignored-dir
I ignored-file
~/src/subversion/svn-status-tests> svn status ignored-dir ignored-file
? ignored-dir
I ignored-file
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.
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?
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?
Log message:
[[[
--------10--------20--------30--------40--------50--------60--------70--------80
Fix status indicator "I": when an ignored directory was given as an explicit
argument, its status was reported as "?".
* subversion/libsvn_wc/status.c
(svn_wc_statuses)
* subversion/tests/clients/cmdline/stat_tests.py
(status_for_unignored_dir) New function.
(test_list) Add the new test.
]]]
- Julian
Re: [PATCH]: status "I" not consistently shown for ignored directory
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>
>>Should I commit this or does it need more work?
>
> It looks reasonable, commit it.
Thanks. Committed in r6637.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: status "I" not consistently shown for ignored
directory
Posted by Philip Martin <ph...@codematters.co.uk>.
Julian Foad <ju...@btopenworld.com> writes:
> Should I commit this or does it need more work?
It looks reasonable, commit it.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: status "I" not consistently shown for ignored directory
Posted by Julian Foad <ju...@btopenworld.com>.
Should I commit this or does it need more work?
- Julian
Julian Foad wrote:
> New version of patch.
>
> I have factored out the duplicated code into a local function, removed
> an unnecessary comment and name change from the test script, and fixed
> the log message.
>
> Log message:
> [[[
> Fix status indicator "I": when an ignored directory was given as an explicit
> argument, its status was reported as "?".
>
> * subversion/libsvn_wc/status.c
> (add_unversioned_path) New function factored out of svn_wc_statuses.
> (svn_wc_statuses) Call add_unversioned_path once where I factored it out,
> and once instead of add_status_structure to report the status of an
> explicitly requested unversioned directory.
> (add_unversioned_item) Fix argument names in the comment.
>
> * subversion/tests/clients/cmdline/stat_tests.py
> (status_for_unignored_file) Test a directory as well as a file. Simplify
> the code by using "run_and_verify_svn".
> ]]]
>
> - Julian
>
>
> ------------------------------------------------------------------------
>
> Index: subversion/libsvn_wc/status.c
> ===================================================================
> --- subversion/libsvn_wc/status.c (revision 6599)
> +++ subversion/libsvn_wc/status.c (working copy)
> @@ -410,18 +410,18 @@
> }
>
>
> -/* Add a status_structure for BASENAME to the STATUSHASH, assuming
> +/* Add a status structure for NAME to the STATUSHASH, assuming
> that the file is unversioned. This function should never
> be called on a versioned entry.
>
> NAME is the basename of the unversioned file whose status is being
> requested.
>
> - PATH_KIND is the node kind of PATH as determined by the caller.
> + PATH_KIND is the node kind of NAME as determined by the caller.
>
> STATUSHASH is a mapping from path to status structure. On entry, it
> may or may not contain status structures for other paths. Upon return
> - it may contain a status structure for BASENAME.
> + it may contain a status structure for NAME.
>
> ADM_ACCESS is an access baton for the working copy path.
>
> @@ -432,7 +432,7 @@
>
> If NO_IGNORE is non-zero, the item will be added regardless of whether
> it is ignored; otherwise we will only add the item if it does not
> - match any of the patterns in IGNORES.
> + match any of the patterns in PATTERNS.
>
> If a status structure for the item is added, NOTIFY_FUNC will called
> with the path of the item and the NOTIFY_BATON. NOTIFY_FUNC may be
> @@ -479,6 +479,36 @@
> return SVN_NO_ERROR;
> }
>
> +/* Add an unversioned item PATH to the given STATUSHASH.
> + This is a convenience wrapper around add_unversioned_item and takes the
> + same parameters except:
> + PATH is the full path; only its base name will be used.
> + DEFAULT_IGNORES will have local ignores added to it.
> + It is assumed that the item is not to be ignored.
> +*/
> +static svn_error_t *
> +add_unversioned_path (const char *path,
> + svn_node_kind_t path_kind,
> + apr_hash_t *statushash,
> + svn_wc_adm_access_t *adm_access,
> + apr_array_header_t *default_ignores,
> + svn_wc_notify_func_t notify_func,
> + void *notify_baton,
> + apr_pool_t *pool)
> +{
> + char *name;
> + apr_array_header_t *patterns;
> +
> + patterns = apr_array_make (pool, 1, sizeof(const char *));
> + SVN_ERR (collect_ignore_patterns (patterns, default_ignores, adm_access,
> + pool));
> +
> + name = svn_path_basename (path, pool);
> + return add_unversioned_item (name, path_kind, statushash, adm_access,
> + patterns, TRUE, notify_func, notify_baton,
> + pool);
> +}
> +
> /* Add all items that are NOT in ENTRIES (which is a list of PATH's
> versioned things) to the STATUSHASH as unversioned items,
>
> @@ -795,19 +825,9 @@
> we're ignoring the GET_ALL flag and unconditionally fetching
> the status structure. */
> if (!entry)
> - {
> - char *name;
> - apr_array_header_t *patterns;
> -
> - patterns = apr_array_make (pool, 1, sizeof(const char *));
> - SVN_ERR (collect_ignore_patterns (patterns, ignores,
> - adm_access, pool));
> -
> - name = svn_path_basename (path, pool);
> - SVN_ERR (add_unversioned_item (name, kind, statushash,
> - adm_access, patterns, TRUE,
> - notify_func, notify_baton, pool));
> - }
> + SVN_ERR (add_unversioned_path (path, kind, statushash, adm_access,
> + ignores, notify_func, notify_baton,
> + pool));
> else
> {
> SVN_ERR (svn_wc_entry (&parent_entry,
> @@ -831,9 +851,8 @@
> /* A wc format of 0 means this directory is not being versioned
> at all (not by Subversion, anyway). */
> if (wc_format_version == 0)
> - return add_status_structure
> - (statushash, path, NULL, NULL, NULL,
> - svn_node_dir, FALSE, FALSE, notify_func, notify_baton, pool);
> + return add_unversioned_path (path, kind, statushash, adm_access,
> + ignores, notify_func, notify_baton, pool);
>
> SVN_ERR (svn_wc_is_wc_root (&is_root, path, adm_access, pool));
> if (! is_root)
> Index: subversion/tests/clients/cmdline/stat_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/stat_tests.py (revision 6599)
> +++ subversion/tests/clients/cmdline/stat_tests.py (working copy)
> @@ -318,7 +318,7 @@
>
>
> def status_for_unignored_file(sbox):
> - "status for unignored file"
> + "status for unignored file and directory"
>
> sbox.build()
>
> @@ -329,33 +329,23 @@
>
> try:
> svntest.main.file_append('newfile', 'this is a new file')
> - svntest.main.run_svn(None, 'propset', 'svn:ignore', 'newfile', '.')
> + os.makedirs('newdir')
> + svntest.main.run_svn(None, 'propset', 'svn:ignore', 'new*', '.')
>
> # status on the 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 +newfile", line):
> - status = 0
> + svntest.actions.run_and_verify_svn(None,
> + [' M .\n',
> + 'I newdir\n',
> + 'I newfile\n'],
> + [],
> + 'status', '--no-ignore', '.')
>
> - if (status == 1):
> - raise svntest.Failure
> -
> # status specifying the file explicitly on the command line
> - stat_output, err_output = svntest.main.run_svn(None, 'status', 'newfile')
> -
> - if err_output:
> - raise svntest.Failure
> - status = 1
> - for line in stat_output:
> - if re.match("I +newfile", line):
> - status = 0
> -
> - if (status == 1):
> - raise svntest.Failure
> + svntest.actions.run_and_verify_svn(None,
> + ['I newdir\n',
> + 'I newfile\n'],
> + [],
> + 'status', 'newdir', 'newfile')
>
> finally:
> os.chdir(was_cwd)
>
>
>
> ------------------------------------------------------------------------
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: status "I" not consistently shown for ignored directory
Posted by Julian Foad <ju...@btopenworld.com>.
New version of patch.
I have factored out the duplicated code into a local function, removed an unnecessary comment and name change from the test script, and fixed the log message.
Log message:
[[[
Fix status indicator "I": when an ignored directory was given as an explicit
argument, its status was reported as "?".
* subversion/libsvn_wc/status.c
(add_unversioned_path) New function factored out of svn_wc_statuses.
(svn_wc_statuses) Call add_unversioned_path once where I factored it out,
and once instead of add_status_structure to report the status of an
explicitly requested unversioned directory.
(add_unversioned_item) Fix argument names in the comment.
* subversion/tests/clients/cmdline/stat_tests.py
(status_for_unignored_file) Test a directory as well as a file. Simplify
the code by using "run_and_verify_svn".
]]]
- Julian
Re: [PATCH]: status "I" not consistently shown for ignored directory
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin wrote:
> Julian Foad <ju...@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?
Thanks for the enlightenment ;-) Seriously, because it is rather few lines (4 statements taking up 11 lines) taking rather many (8) parameters, I am not sure whether that would be better, and would be glad of your opinion.
>>+ status = 1
>>+ for line in stat_output:
>>+ if re.match("I +newdir", line):
>>+ status = 0
...
> An RE is probably over the top as the number of spaces should be
> known. When I wrote something like that someone (Greg?) suggested
This is all copied from the existing test above it. I should have said: my knowledge of Python and of the svn test suite were zero recently and I am only learning by example. But I am glad to learn more from you.
> for line in stat_output:
> if line.find("I newdir") != -1:
> break
...
> 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')
That all looks much better. I understand the principle of "run_and_verify_status" but not the details of how to use it. "run_and_verify_svn" is easy and reasonably appropriate. Now I can combine the two tests, and have the combined version neater than the old single test.
Patch re-attached with the main code untouched but the test script much improved.
Log message:
[[[
Fix status indicator "I": when an ignored directory was given as an explicit
argument, its status was reported as "?".
* subversion/libsvn_wc/status.c
(svn_wc_statuses)
* subversion/tests/clients/cmdline/stat_tests.py
(status_for_unignored_dir) New function.
(test_list) Add the new test.
]]]
- Julian
Re: [PATCH]: status "I" not consistently shown for ignored
directory
Posted by Philip Martin <ph...@codematters.co.uk>.
Julian Foad <ju...@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