> 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
Received on 2008-12-05 13:27:43 CET