The attached brute-force patch seems to fix many of the cases where changelist
filtering was missing or wrong, but I don't much like it and haven't properly
tested it.
I'm attaching it for us to consider, but not necessarily to commit. If anyone
wants to kick it around a bit (test it) and report back, or preferably write
some proper tests for it, that would be great.
Bert Huijben wrote:
> Julian Foad wrote:
>> (In the longer term, I would like it if we could implement this changelist
>> filtering in one place instead of scattering these "if" conditions around.
>> I would like to see a code structure more analogous to "find /wc/path <node
>> selection conditions: depth, changelists, etc.> | xargs diff", probably using
>> the existing svn_wc__internal_walk_children() function or something similar
>> to it.)
>
> You do know that this kind of restructuring would make it +- impossible to
> handle tree changes?
[...]
No, I don't know that.
> Personally I would wish we could just do the changelist filtering in the
> output layer, instead of in all the diff drivers separately... But the
> current code doesn't properly guarantee access to the local working copy
> paths to do that kind of processing there.
>
> In many cases it prefers to just pass something url like. (Not necessary the
> proper url)
If filtering at the output layer is easier that's totally fine with me,
anything as long as it's simple and consistent.
Can we say the same thing in a more positive way: "It should be possible to do
the filtering in the output layer. One thing that needs to be fixed to make
this possible is that the WC paths need to be communicated consistently."
- Julian
Fix 'changelist' filtering in repos-WC and WC-WC diffs.
When a changelist was specified, the diff code was failing to filter out
several kinds of changes, especially adds and deletes and directory property
changes. In one common case when a changelist was specified, it crashed
(reported by Peter Galcik).
Note: I'm not confident that this patch covers all possible cases. It needs
some tests, and preferably needs to be rewritten in a more modular form.
* subversion/libsvn_wc/diff_editor.c
(svn_wc__diff_base_working_diff): Fix a crash when attempting to use a
changelist filter when the file is not a member of any changelist.
(walk_local_nodes_diff): Apply changelist filtering to deletes.
(svn_wc__diff_local_only_file): Fix changelist filtering: skip the locally
added file if it is not a member of any changelist when changelist
filtering is active.
(svn_wc__diff_local_only_dir,
svn_wc__diff_base_only_file,
svn_wc__diff_base_only_dir): Implement changelist filtering.
* subversion/libsvn_wc/diff.h
(svn_wc__diff_base_only_file,
svn_wc__diff_base_only_dir): Support changelist filtering.
* subversion/libsvn_wc/diff_local.c
(diff_status_callback): Apply changelist filtering to repository-only
nodes.
--This line, and those below, will be ignored--
Index: subversion/libsvn_wc/diff_editor.c
===================================================================
--- subversion/libsvn_wc/diff_editor.c (revision 1621760)
+++ subversion/libsvn_wc/diff_editor.c (working copy)
@@ -432,13 +432,14 @@ svn_wc__diff_base_working_diff(svn_wc__d
assert(status == svn_wc__db_status_normal
|| status == svn_wc__db_status_added
|| (status == svn_wc__db_status_deleted && diff_pristine));
/* If the item is not a member of a specified changelist (and there are
some specified changelists), skip it. */
- if (changelist_hash && !svn_hash_gets(changelist_hash, changelist))
+ if (changelist_hash
+ && (!changelist || !svn_hash_gets(changelist_hash, changelist)))
return SVN_NO_ERROR;
if (status != svn_wc__db_status_normal)
{
SVN_ERR(svn_wc__db_base_get_info(&base_status, NULL, &db_revision,
@@ -789,18 +790,20 @@ walk_local_nodes_diff(struct edit_baton_
{
/* Report repository form deleted */
if (base_kind == svn_node_file && diff_files)
SVN_ERR(svn_wc__diff_base_only_file(db, child_abspath,
child_relpath, eb->revnum,
eb->processor, dir_baton,
+ eb->changelist_hash,
iterpool));
else if (base_kind == svn_node_dir && diff_dirs)
SVN_ERR(svn_wc__diff_base_only_dir(db, child_abspath,
child_relpath, eb->revnum,
depth_below_here,
eb->processor, dir_baton,
+ eb->changelist_hash,
eb->cancel_func,
eb->cancel_baton,
iterpool));
}
else if (!local_only) /* Not local only nor remote only */
{
@@ -944,14 +947,14 @@ svn_wc__diff_local_only_file(svn_wc__db_
assert(kind == svn_node_file
&& (status == svn_wc__db_status_normal
|| status == svn_wc__db_status_added
|| (status == svn_wc__db_status_deleted && diff_pristine)));
- if (changelist && changelist_hash
- && !svn_hash_gets(changelist_hash, changelist))
+ if (changelist_hash
+ && (!changelist || !svn_hash_gets(changelist_hash, changelist)))
return SVN_NO_ERROR;
if (status == svn_wc__db_status_deleted)
{
assert(diff_pristine);
@@ -1124,12 +1127,13 @@ svn_wc__diff_local_only_dir(svn_wc__db_t
NULL,
right_src,
copyfrom_src,
processor_parent_baton,
processor,
scratch_pool, iterpool));
+ /* ### skip_children is not used */
SVN_ERR(svn_wc__db_read_children_info(&nodes, &conflicts, db, local_abspath,
FALSE /* base_tree_only */,
scratch_pool, iterpool));
if (depth_below_here == svn_depth_immediates)
@@ -1195,30 +1199,42 @@ svn_wc__diff_local_only_dir(svn_wc__db_t
break;
}
}
if (!skip)
{
- apr_hash_t *right_props;
- if (diff_pristine)
- SVN_ERR(svn_wc__db_read_pristine_props(&right_props, db, local_abspath,
- scratch_pool, scratch_pool));
- else
- SVN_ERR(svn_wc__get_actual_props(&right_props, db, local_abspath,
- scratch_pool, scratch_pool));
+ if (!changelist_hash)
+ {
+ apr_hash_t *right_props;
+ if (diff_pristine)
+ SVN_ERR(svn_wc__db_read_pristine_props(&right_props, db, local_abspath,
+ scratch_pool, scratch_pool));
+ else
+ SVN_ERR(svn_wc__get_actual_props(&right_props, db, local_abspath,
+ scratch_pool, scratch_pool));
- SVN_ERR(processor->dir_added(relpath,
- copyfrom_src,
- right_src,
- copyfrom_src
- ? pristine_props
- : NULL,
- right_props,
- pdb,
- processor,
- iterpool));
+ SVN_ERR(processor->dir_added(relpath,
+ copyfrom_src,
+ right_src,
+ copyfrom_src
+ ? pristine_props
+ : NULL,
+ right_props,
+ pdb,
+ processor,
+ iterpool));
+ }
+ else
+ {
+ SVN_ERR(processor->dir_closed(relpath,
+ NULL,
+ right_src,
+ pdb,
+ processor,
+ iterpool));
+ }
}
svn_pool_destroy(iterpool);
return SVN_NO_ERROR;
}
@@ -1306,18 +1322,20 @@ svn_error_t *
svn_wc__diff_base_only_file(svn_wc__db_t *db,
const char *local_abspath,
const char *relpath,
svn_revnum_t revision,
const svn_diff_tree_processor_t *processor,
void *processor_parent_baton,
+ apr_hash_t *changelist_hash,
apr_pool_t *scratch_pool)
{
svn_wc__db_status_t status;
svn_node_kind_t kind;
const svn_checksum_t *checksum;
apr_hash_t *props;
+ const char *changelist;
void *file_baton = NULL;
svn_boolean_t skip = FALSE;
svn_diff_source_t *left_src;
const char *pristine_file;
SVN_ERR(svn_wc__db_base_get_info(&status, &kind,
@@ -1329,12 +1347,23 @@ svn_wc__diff_base_only_file(svn_wc__db_t
scratch_pool, scratch_pool));
SVN_ERR_ASSERT(status == svn_wc__db_status_normal
&& kind == svn_node_file
&& checksum);
+ SVN_ERR(svn_wc__db_read_info(NULL, NULL, NULL, NULL, NULL, NULL,
+ NULL, NULL, NULL, NULL, NULL, NULL,
+ NULL, NULL, NULL, NULL, NULL, NULL,
+ NULL, &changelist, NULL, NULL, NULL,
+ NULL, NULL, NULL, NULL,
+ db, local_abspath,
+ scratch_pool, scratch_pool));
+ if (changelist_hash
+ && (!changelist || !svn_hash_gets(changelist_hash, changelist)))
+ return SVN_NO_ERROR;
+
left_src = svn_diff__source_create(revision, scratch_pool);
SVN_ERR(processor->file_opened(&file_baton, &skip,
relpath,
left_src,
NULL /* right_src */,
@@ -1366,12 +1395,13 @@ svn_wc__diff_base_only_dir(svn_wc__db_t
const char *local_abspath,
const char *relpath,
svn_revnum_t revision,
svn_depth_t depth,
const svn_diff_tree_processor_t *processor,
void *processor_parent_baton,
+ apr_hash_t *changelist_hash,
svn_cancel_func_t cancel_func,
void *cancel_baton,
apr_pool_t *scratch_pool)
{
void *dir_baton = NULL;
svn_boolean_t skip = FALSE;
@@ -1435,12 +1465,13 @@ svn_wc__diff_base_only_dir(svn_wc__db_t
case svn_node_file:
case svn_node_symlink:
SVN_ERR(svn_wc__diff_base_only_file(db, child_abspath,
child_relpath,
revision,
processor, dir_baton,
+ changelist_hash,
iterpool));
break;
case svn_node_dir:
if (depth > svn_depth_files || depth == svn_depth_unknown)
{
svn_depth_t depth_below_here = depth;
@@ -1450,12 +1481,13 @@ svn_wc__diff_base_only_dir(svn_wc__db_t
SVN_ERR(svn_wc__diff_base_only_dir(db, child_abspath,
child_relpath,
revision,
depth_below_here,
processor, dir_baton,
+ changelist_hash,
cancel_func,
cancel_baton,
iterpool));
}
break;
@@ -1464,22 +1496,34 @@ svn_wc__diff_base_only_dir(svn_wc__db_t
}
}
}
if (!skip)
{
- apr_hash_t *props;
- SVN_ERR(svn_wc__db_base_get_props(&props, db, local_abspath,
- scratch_pool, scratch_pool));
+ if (!changelist_hash)
+ {
+ apr_hash_t *props;
+ SVN_ERR(svn_wc__db_base_get_props(&props, db, local_abspath,
+ scratch_pool, scratch_pool));
- SVN_ERR(processor->dir_deleted(relpath,
- left_src,
- props,
- dir_baton,
- processor,
- scratch_pool));
+ SVN_ERR(processor->dir_deleted(relpath,
+ left_src,
+ props,
+ dir_baton,
+ processor,
+ scratch_pool));
+ }
+ else
+ {
+ SVN_ERR(processor->dir_closed(relpath,
+ left_src,
+ NULL,
+ dir_baton,
+ processor,
+ scratch_pool));
+ }
}
return SVN_NO_ERROR;
}
/* An svn_delta_editor_t function. */
Index: subversion/libsvn_wc/diff.h
===================================================================
--- subversion/libsvn_wc/diff.h (revision 1621760)
+++ subversion/libsvn_wc/diff.h (working copy)
@@ -100,12 +100,13 @@ svn_error_t *
svn_wc__diff_base_only_file(svn_wc__db_t *db,
const char *local_abspath,
const char *relpath,
svn_revnum_t revision,
const svn_diff_tree_processor_t *processor,
void *processor_parent_baton,
+ apr_hash_t *changelist_hash,
apr_pool_t *scratch_pool);
/* Reports the BASE-directory LOCAL_ABSPATH and everything below it (limited
by DEPTH) as deleted to PROCESSOR with relpath RELPATH and parent baton
PROCESSOR_PARENT_BATON.
@@ -117,12 +118,13 @@ svn_wc__diff_base_only_dir(svn_wc__db_t
const char *local_abspath,
const char *relpath,
svn_revnum_t revision,
svn_depth_t depth,
const svn_diff_tree_processor_t *processor,
void *processor_parent_baton,
+ apr_hash_t *changelist_hash,
svn_cancel_func_t cancel_func,
void *cancel_baton,
apr_pool_t *scratch_pool);
/* Diff the file PATH against the text base of its BASE layer. At this
* stage we are dealing with a file that does exist in the working copy.
Index: subversion/libsvn_wc/diff_local.c
===================================================================
--- subversion/libsvn_wc/diff_local.c (revision 1621760)
+++ subversion/libsvn_wc/diff_local.c (working copy)
@@ -335,20 +335,22 @@ diff_status_callback(void *baton,
if (base_kind == svn_node_file)
SVN_ERR(svn_wc__diff_base_only_file(db, child_abspath,
child_relpath,
SVN_INVALID_REVNUM,
eb->processor,
eb->cur ? eb->cur->baton : NULL,
+ eb->changelist_hash,
scratch_pool));
else if (base_kind == svn_node_dir)
SVN_ERR(svn_wc__diff_base_only_dir(db, child_abspath,
child_relpath,
SVN_INVALID_REVNUM,
depth_below_here,
eb->processor,
eb->cur ? eb->cur->baton : NULL,
+ eb->changelist_hash,
eb->cancel_func,
eb->cancel_baton,
scratch_pool));
}
else if (!local_only)
{
