Brian Denny <brian@briandenny.net> writes:
> I had to do a bit more reworking than i'd anticipated, because
> the code path which checks whether a file is ignored was being
> bypassed in the case where the file was given explicitly on the command
> line. I got around this by doing a little refactoring in
> libsvn_wc/status.c ... let me know if you think there's a better
> solution.
>
> looking forward to your critique,
Wow. This was a non-trivial amount of work. Looks excellent to me,
overall; a few small comments below:
> Log:
>
> Resolve Issue #1076.
It would be good to summarize the change here, not just give the issue
number (particularly with an issue like this, whose summary is just
"svn status corner cases", which won't help someone understand what
effects the change actually has).
> * subversion/include/svn_wc.h
> (enum svn_wc_status_kind): Add status kind svn_wc_status_ignored.
>
> * subversion/libsvn_wc/status.c
> (assemble_status): Set text_status to svn_wc_status_ignored
> for ignored files.
> (collect_ignore_patterns): New function, extracted from
> add_unversioned_items.
> (add_unversioned_item): New function, extracted from
> add_unversioned_items.
> (add_unversioned_items): Extract helper functions mentioned above.
> (svn_wc_statuses): If there is no entry for PATH, handle it
> correctly as an unversioned item.
>
> * subversion/clients/cmdline/status.c
> (generate_status_code): Return 'I' for svn_wc_status_ignored.
> (svn_cl__print_status_list): Skip printing items whose status is
> svn_wc_status_none.
>
> * subversion/clients/cmdline/main.c
> (svn_cl__cmd_table): Add 'I' to output of 'svn help status'.
>
> * subversion/tests/clients/cmdline/stat_tests.py
> (status_for_unignored_file)
> [renamed from status_blank_for_unignored_file]: Check that text
> status is 'I' for a file marked ignored, if specified explicitly
> or --no-ignores is given.
> (status_for_nonexistent_file): Check that no output is given for a
> file that is not versioned and does not exist.
>
> * doc/book/book/ch06.xml
> Give example of using 'svn status --no-ignore'
>
> * doc/book/book/ch08.xml
> Add explanation of 'I' to 'svn status' reference
The list of changes itself is really clear, thanks for being so
thorough. (And new regression tests & docs, oh, what a bonus!)
> +/* Store in PATTERNS a list of all svn:ignore properties from
> + the working copy directory, including the default ignores
> + passed in as IGNORES.
> +*/
> +static svn_error_t *
> +collect_ignore_patterns (apr_array_header_t *patterns,
> + apr_array_header_t *ignores,
> + svn_wc_adm_access_t *adm_access,
> + apr_pool_t *pool)
I know it's a pain, but when you create a new function, it should
explain all of its parameters. For example, can ADM_ACCESS be null or
not? What precisely gets allocated in POOL? (And in what precise
form is the data in PATTERNS stored?)
> +/* Add a status_structure for ITEM to the STATUSHASH, assuming
> + that the ITEM is unversioned. This function should never
> + be called on a versioned entry. */
> +static svn_error_t *
> +add_unversioned_item (const char *item,
> + svn_node_kind_t path_kind,
> + apr_hash_t *statushash,
> + svn_wc_adm_access_t *adm_access,
> + apr_array_header_t *patterns,
> + svn_boolean_t no_ignore,
> + svn_wc_notify_func_t notify_func,
> + void *notify_baton,
> + apr_pool_t *pool)
Same for this function, in a big way :-).
> - /* Copy default ignores into the local PATTERNS array. */
> - patterns = apr_array_make (subpool, 1, sizeof(const char *));
> - for (i = 0; i < ignores->nelts; i++)
> - {
> - const char *ignore = APR_ARRAY_IDX (ignores, i, const char *);
> - (*((const char **) apr_array_push (patterns))) = ignore;
> - }
> -
> - /* Then add any svn:ignore globs to the PATTERNS array. */
> - SVN_ERR (add_ignore_patterns (adm_access, patterns, subpool));
> + patterns = apr_array_make (pool, 1, sizeof(const char *));
> + SVN_ERR (collect_ignore_patterns (patterns, ignores,
> + adm_access, subpool));
Hmmm. I'm not sure there's anything wrong here, but a minor red flag
went up when I saw that `patterns' is allocated in `pool', but
collect_ignore_patterns() takes `subpool', not `pool'. It seems like
the lifetime effects in the new code are different from the old code,
since the old code (visible above) just pushes stuff straight from
`ignores' to `patterns', without involving a subpool.
If the new code is correct, perhaps you might want a comment
explaining what's going on (which will go nicely with that doc string
explaining the `pool' parameter of collect_ignore_parameters() :-) ).
> - /* Read the appropriate entries file */
> + /* Read the default ignores from the config files. */
> + apr_array_header_t *ignores;
> + SVN_ERR (svn_wc_get_default_ignores (&ignores, config, pool));
The new comment implies that we're actually reading in config files
here. We're not (which is good, since that would be a big no-no down
here in libsvn_wc); imho the comment could either go away or get
tweaked, your call.
> - /* Read the default ignores from the config files. */
> - SVN_ERR (svn_wc_get_default_ignores (&ignores, config, pool));
Aha. I see that it was in the code before, and was not written by
you, just caught in a cut-and-paste -- sorry :-). Feel free to fix
stuff like that as you go, though.
> - if re.match(" +newfile", line):
> + if re.match("I +newfile", line):
> status = 0
> +
> + # status specifying the file explicitly on the command line
> + stat_output, err_output = svntest.main.run_svn(None, 'status', 'newfile')
> +
> + if err_output:
> + return 1
> + status = 1
> + for line in stat_output:
> + if re.match("I +newfile", line):
> + status = 0
>
> os.chdir(was_cwd)
>
> return status
I realize this is new (possibly happened after your patch?) but Brane
just made a change (revision 5321, see also issue #1192) that makes
the testsuite use exceptions instead of returns to signify failure.
Instead of 'return 1', you want 'raise svntest.Failure' now. And many
standard helper functions will raise the exception themselves, so for
example, for sbox.build(), the caller doesn't have to do any error
checking at all.
It would be good if new code could use only the new system, from now
on, so our conversion task never gets any bigger than it is today.
> +def status_for_nonexistent_file(sbox):
> + "status for a file neither on disk nor under version control"
> +
> + if sbox.build():
> + return 1
> +
> + [...etc...]
Same here, of course.
That's it. Nice work! One more round, and then we can apply, imho...
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Mar 18 06:03:17 2003