On 9/4/07, Erik Huelsmann <ehuels@gmail.com> wrote:
> On 9/4/07, Ivan Zhakov <chemodax@gmail.com> wrote:
> > Hi Erik,
> >
> > On 9/3/07, Erik Huelsmann <ehuels@gmail.com> wrote:
> > > I found this code at the top of revert_admin_things()
> > > (libsvn_wc/adm_ops.c). It's about reverting the properties on replaced
> > > files (as part of reverting replaced files).
> > >
> > > 1731 /* Deal with properties. */
> > > 1732 if (entry->schedule == svn_wc_schedule_replace)
> > > 1733 {
> > > 1734 const char *rprop;
> > > 1735 svn_node_kind_t kind;
> > > 1736
> > > 1737 revert_base = TRUE;
> > > 1738 /* Use the revertpath as the new propsbase if it exists. */
> > > 1739 SVN_ERR(svn_wc__prop_revert_path(&rprop, fullpath,
> > > entry->kind, FALSE,
> > > 1740 pool));
> > > 1741 SVN_ERR(svn_io_check_path(rprop, &kind, pool));
> > > 1742
> > > 1743 /* If the revert-base doesn't exist, check for a normal propsbase */
> > > 1744 if (kind != svn_node_file)
> > > 1745 {
> > > 1746 SVN_ERR(svn_wc__prop_base_path(&rprop, fullpath,
> > > entry->kind, FALSE,
> > > 1747 pool));
> > > 1748 SVN_ERR(svn_io_check_path(rprop, &kind, pool));
> > > 1749 revert_base = FALSE;
> > > 1750 }
> > > 1751 if (kind == svn_node_file)
> > > 1752 {
> > > 1753 baseprops = apr_hash_make(pool);
> > > 1754 SVN_ERR(svn_wc__load_prop_file(rprop, baseprops, pool));
> > > 1755 /* Ensure the revert propfile gets removed. */
> > > 1756 if (revert_base)
> > > 1757 SVN_ERR(svn_wc__loggy_remove(&log_accum, adm_access,
> > > rprop, pool));
> > > 1758 *reverted = TRUE;
> > > 1759 }
> > > 1760 }
> > >
> > >
> > > Now, what I think is wrong - but this is what I'm asking your opinions
> > > on - is: the code uses the base props if there are no revert props.
> > > However, if there are no revert props, something is wrong: even if
> > > there are no base props, when creating the revert files, we explicitly
> > > create a revert file for the 'no props' case.
> > >
> > > But say there's no revert file, then the code proceeds to use the base
> > > props. Isn't that incorrect in all cases? I mean: in case of a
> > > replacement-without-history, there are no base props, but in case of a
> > > replacement-with-history these base props belong to the replacing
> > > object (not to the replaced object)...
> > In case of replacement-without-history there ARE base props.
>
> Ah, ok. that makes things a lot more confusing (to me). Then I think
> the code is correct but not strict enough in checking the
> pre-conditions. It should *only* check for revert-props in the
> replace-with-history case and *only* check for the base-props in the
> replace-without-history case. Agreed?
I like this approach. I didn't like that we make decision on file
presence, instead of checking conditions when file should exists or
don't.
>
> If you agree, I'll make the change as part of a larger
> 'Trying-to-clean-up' operation.
Ok!
--
Ivan Zhakov
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Sep 4 09:45:49 2007