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 2008/10/29 12:09:05 UTC
Redundant lines in do_entry_deletion()?
Can anyone see whether the following marked lines are redundant? It
looks to me like they are just re-setting the state of the existing
entry.
- Julian
trunk/subversion/libsvn_wc/update_editor.c
> @@ -1359,13 +1359,13 @@ do_entry_deletion(struct edit_baton *eb,
>
> tmp_entry.revision = *(eb->target_revision);
> tmp_entry.kind =
> - (entry->kind == svn_node_file) ? svn_node_file : svn_node_dir; /* ### redundant? */
> tmp_entry.deleted = TRUE;
>
> SVN_ERR(svn_wc__loggy_entry_modify(&log_item, adm_access,
> full_path, &tmp_entry,
> SVN_WC__ENTRY_MODIFY_REVISION
> - | SVN_WC__ENTRY_MODIFY_KIND /* ### redundant change? */
> | SVN_WC__ENTRY_MODIFY_DELETED,
> pool));
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Redundant lines in do_entry_deletion()?
Posted by "C. Michael Pilato" <cm...@collab.net>.
Julian Foad wrote:
> Can anyone see whether the following marked lines are redundant? It
> looks to me like they are just re-setting the state of the existing
> entry.
>
> - Julian
>
> trunk/subversion/libsvn_wc/update_editor.c
>
>> @@ -1359,13 +1359,13 @@ do_entry_deletion(struct edit_baton *eb,
>>
>> tmp_entry.revision = *(eb->target_revision);
>> tmp_entry.kind =
>> - (entry->kind == svn_node_file) ? svn_node_file : svn_node_dir; /* ### redundant? */
>> tmp_entry.deleted = TRUE;
>>
>> SVN_ERR(svn_wc__loggy_entry_modify(&log_item, adm_access,
>> full_path, &tmp_entry,
>> SVN_WC__ENTRY_MODIFY_REVISION
>> - | SVN_WC__ENTRY_MODIFY_KIND /* ### redundant change? */
>> | SVN_WC__ENTRY_MODIFY_DELETED,
>> pool));
>>
Certainly looks redundant from where I sit. Does the history of this region
of code reveal anything interesting? Has the redundancy evolved into place,
or was it composed originally in this way? Is it significant that the first
of your marked lines doesn't just read:
tmp_entry.kind = entry->kind;
? Is there another state that entry->kind could hold here besides
svn_node_file or svn_node_dir?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: Redundant lines in do_entry_deletion()?
Posted by Julian Foad <ju...@btopenworld.com>.
> Julian Foad wrote:
> > Can anyone see whether the following marked lines are redundant? It
> > looks to me like they are just re-setting the state of the existing
> > entry.
> > trunk/subversion/libsvn_wc/update_editor.c
> >
> >> @@ -1359,13 +1359,13 @@ do_entry_deletion(struct edit_baton *eb,
> >>
> >> tmp_entry.revision = *(eb->target_revision);
> >> tmp_entry.kind =
> >> - (entry->kind == svn_node_file) ? svn_node_file : svn_node_dir; /* ### redundant? */
> >> tmp_entry.deleted = TRUE;
> >>
> >> SVN_ERR(svn_wc__loggy_entry_modify(&log_item, adm_access,
> >> full_path, &tmp_entry,
> >> SVN_WC__ENTRY_MODIFY_REVISION
> >> - | SVN_WC__ENTRY_MODIFY_KIND /* ### redundant change? */
> >> | SVN_WC__ENTRY_MODIFY_DELETED,
C. Michael Pilato wrote:
> Certainly looks redundant from where I sit. Does the history of this region
> of code reveal anything interesting? Has the redundancy evolved into place,
> or was it composed originally in this way? Is it significant that the first
> of your marked lines doesn't just read:
>
> tmp_entry.kind = entry->kind;
>
> ? Is there another state that entry->kind could hold here besides
> svn_node_file or svn_node_dir?
I found the original code in r6748:
+ (kind == svn_node_file) ?
+ SVN_WC__ENTRIES_ATTR_FILE_STR :
+ SVN_WC__ENTRIES_ATTR_DIR_STR,
So it looks like it was just not optimised when the operands were later
turned into "svn_node_file" and "svn_node_dir".
Neels J. Hofmeyr wrote:
> Yeah, actually I saw that recently and actually I found a partial answer: It
> *is* necessary to set the entry kind during svn_wc__loggy_entry_modify(),
> because (IIUC) this code in some cases adds an entry where there should be
> one. So if we don't set the kind, there will be an un-kinded entry or
> something. Let's take a quick look...
>
> trunk r33956: subversion/libsvn_wc/update_editor.c: 1430
> [[[
> /* If the thing being deleted is the *target* of this update, then
> we need to recreate a 'deleted' entry, so that parent can give
> accurate reports about itself in the future. */
> if (strcmp(path, eb->target) == 0)
> {
> svn_wc_entry_t tmp_entry;
>
> tmp_entry.revision = *(eb->target_revision);
> tmp_entry.kind =
> (entry->kind == svn_node_file) ? svn_node_file : svn_node_dir; /*
> ### redundant? */
> tmp_entry.deleted = TRUE;
>
> SVN_ERR(svn_wc__loggy_entry_modify(&log_item, adm_access,
> full_path, &tmp_entry,
> SVN_WC__ENTRY_MODIFY_REVISION
> | SVN_WC__ENTRY_MODIFY_KIND /* ###
> redundant change? */
> | SVN_WC__ENTRY_MODIFY_DELETED,
> pool));
>
> eb->target_deleted = TRUE;
> }
> ]]]
>
> The comment says something about possibly re-creating an entry for an
> explicit target. The only thing I then don't understand is (as cmpilato
> pointed out) why there is this defaulting to svn_node_dir.
>
> BTW, a similar thing is found just above:
>
> trunk r33956: subversion/libsvn_wc/update_editor.c: 1389
> [[[
> /* If the thing being deleted is the *target* of this update, then
> we may need to recreate an entry, so that the parent can give
> accurate reports about itself in future. */
> if (strcmp(path, eb->target) == 0)
> {
> svn_wc_entry_t tmp_entry;
>
> tmp_entry.revision = *(eb->target_revision);
> tmp_entry.kind =
> (entry->kind == svn_node_file) ? svn_node_file : svn_node_dir;
>
> SVN_ERR(svn_wc__loggy_entry_modify(&log_item, adm_access,
> full_path, &tmp_entry,
> SVN_WC__ENTRY_MODIFY_REVISION
> | SVN_WC__ENTRY_MODIFY_KIND,
> pool));
> ]]]
That block is gone now.
> So I say leave the
> | SVN_WC__ENTRY_MODIFY_KIND,
> but investigate whether the
> (entry->kind == svn_node_file) ? svn_node_file : svn_node_dir;
> should just read
> entry->kind;
I have done that and removed the question-comments in r34573.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=980056
Re: Redundant lines in do_entry_deletion()?
Posted by "Neels J. Hofmeyr" <ne...@elego.de>.
Julian Foad wrote:
> Can anyone see whether the following marked lines are redundant? It
> looks to me like they are just re-setting the state of the existing
> entry.
>
> - Julian
>
> trunk/subversion/libsvn_wc/update_editor.c
>
>> @@ -1359,13 +1359,13 @@ do_entry_deletion(struct edit_baton *eb,
>>
>> tmp_entry.revision = *(eb->target_revision);
>> tmp_entry.kind =
>> - (entry->kind == svn_node_file) ? svn_node_file : svn_node_dir; /* ### redundant? */
>> tmp_entry.deleted = TRUE;
>>
>> SVN_ERR(svn_wc__loggy_entry_modify(&log_item, adm_access,
>> full_path, &tmp_entry,
>> SVN_WC__ENTRY_MODIFY_REVISION
>> - | SVN_WC__ENTRY_MODIFY_KIND /* ### redundant change? */
>> | SVN_WC__ENTRY_MODIFY_DELETED,
Yeah, actually I saw that recently and actually I found a partial answer: It
*is* necessary to set the entry kind during svn_wc__loggy_entry_modify(),
because (IIUC) this code in some cases adds an entry where there should be
one. So if we don't set the kind, there will be an un-kinded entry or
something. Let's take a quick look...
trunk r33956: subversion/libsvn_wc/update_editor.c: 1430
[[[
/* If the thing being deleted is the *target* of this update, then
we need to recreate a 'deleted' entry, so that parent can give
accurate reports about itself in the future. */
if (strcmp(path, eb->target) == 0)
{
svn_wc_entry_t tmp_entry;
tmp_entry.revision = *(eb->target_revision);
tmp_entry.kind =
(entry->kind == svn_node_file) ? svn_node_file : svn_node_dir; /*
### redundant? */
tmp_entry.deleted = TRUE;
SVN_ERR(svn_wc__loggy_entry_modify(&log_item, adm_access,
full_path, &tmp_entry,
SVN_WC__ENTRY_MODIFY_REVISION
| SVN_WC__ENTRY_MODIFY_KIND /* ###
redundant change? */
| SVN_WC__ENTRY_MODIFY_DELETED,
pool));
eb->target_deleted = TRUE;
}
]]]
The comment says something about possibly re-creating an entry for an
explicit target. The only thing I then don't understand is (as cmpilato
pointed out) why there is this defaulting to svn_node_dir.
BTW, a similar thing is found just above:
trunk r33956: subversion/libsvn_wc/update_editor.c: 1389
[[[
/* If the thing being deleted is the *target* of this update, then
we may need to recreate an entry, so that the parent can give
accurate reports about itself in future. */
if (strcmp(path, eb->target) == 0)
{
svn_wc_entry_t tmp_entry;
tmp_entry.revision = *(eb->target_revision);
tmp_entry.kind =
(entry->kind == svn_node_file) ? svn_node_file : svn_node_dir;
SVN_ERR(svn_wc__loggy_entry_modify(&log_item, adm_access,
full_path, &tmp_entry,
SVN_WC__ENTRY_MODIFY_REVISION
| SVN_WC__ENTRY_MODIFY_KIND,
pool));
]]]
So I say leave the
| SVN_WC__ENTRY_MODIFY_KIND,
but investigate whether the
(entry->kind == svn_node_file) ? svn_node_file : svn_node_dir;
should just read
entry->kind;
~Neels