Re: Redundant lines in do_entry_deletion()?

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