You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Erik Huelsmann <eh...@gmail.com> on 2007/09/03 09:53:40 UTC
Property-revert of replaced files? Bug?
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)...
Comments?
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Property-revert of replaced files? Bug?
Posted by Karl Fogel <kf...@red-bean.com>.
"Erik Huelsmann" <eh...@gmail.com> writes:
> 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)...
>
> Comments?
Your reasoning makes sense to me, but I couldn't make the problem
happen with the following script:
#!/bin/sh
SVNDIR=/home/kfogel/src/subversion
SVN=${SVNDIR}/subversion/svn/svn
SVNADMIN=${SVNDIR}/subversion/svnadmin/svnadmin
URL=file:///`pwd`/repos
rm -rf repos wc import-me
${SVNADMIN} create repos
echo "### Making a Greek Tree for import..."
mkdir import-me
mkdir import-me/trunk
mkdir import-me/tags
mkdir import-me/branches
mkdir import-me/trunk/A
mkdir import-me/trunk/A/B/
mkdir import-me/trunk/A/C/
mkdir import-me/trunk/A/D/
mkdir import-me/trunk/A/B/E/
mkdir import-me/trunk/A/B/F/
mkdir import-me/trunk/A/D/G/
mkdir import-me/trunk/A/D/H/
echo "This is the file 'iota'." > import-me/trunk/iota
echo "This is the file 'A/mu'." > import-me/trunk/A/mu
echo "This is the file 'A/B/lambda'." > import-me/trunk/A/B/lambda
echo "This is the file 'A/B/E/alpha'." > import-me/trunk/A/B/E/alpha
echo "This is the file 'A/B/E/beta'." > import-me/trunk/A/B/E/beta
echo "This is the file 'A/D/gamma'." > import-me/trunk/A/D/gamma
echo "This is the file 'A/D/G/pi'." > import-me/trunk/A/D/G/pi
echo "This is the file 'A/D/G/rho'." > import-me/trunk/A/D/G/rho
echo "This is the file 'A/D/G/tau'." > import-me/trunk/A/D/G/tau
echo "This is the file 'A/D/H/chi'." > import-me/trunk/A/D/H/chi
echo "This is the file 'A/D/H/omega'." > import-me/trunk/A/D/H/omega
echo "This is the file 'A/D/H/psi'." > import-me/trunk/A/D/H/psi
echo "### Done."
echo ""
echo "### Importing it..."
(cd import-me; ${SVN} import -q -m "Initial import." ${URL})
echo "### Done."
echo ""
echo "### Running the scenario (this might take a while):"
echo ""
${SVN} co -q ${URL}/trunk wc
cd wc
${SVN} propset -q "orig-prop" "orig-val" iota
${SVN} propset -q "newcomer-prop" "newcomer-val" A/mu
${SVN} ci -q -m "Committing new properties."
${SVN} up -q # just for kicks
${SVN} rm -q iota
${SVN} cp -q ${URL}/trunk/A/mu iota
echo "### Here are the properties after replacement:"
echo ""
${SVN} proplist -v iota
echo ""
echo "### Okay, reverting the replacement..."
${SVN} revert -q iota
echo "### Done."
echo ""
echo "### Here are the original properties, after the replacement"
echo "### has been reverted:"
echo ""
${SVN} proplist -v iota
echo ""
cd ..
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Property-revert of replaced files? Bug?
Posted by Ivan Zhakov <ch...@gmail.com>.
On 9/4/07, Erik Huelsmann <eh...@gmail.com> wrote:
> On 9/4/07, Ivan Zhakov <ch...@gmail.com> wrote:
> > Hi Erik,
> >
> > On 9/3/07, Erik Huelsmann <eh...@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
Re: Property-revert of replaced files? Bug?
Posted by Erik Huelsmann <eh...@gmail.com>.
On 9/4/07, Ivan Zhakov <ch...@gmail.com> wrote:
> Hi Erik,
>
> On 9/3/07, Erik Huelsmann <eh...@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?
If you agree, I'll make the change as part of a larger
'Trying-to-clean-up' operation.
Thanks for having a look!
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Property-revert of replaced files? Bug?
Posted by Ivan Zhakov <ch...@gmail.com>.
Hi Erik,
On 9/3/07, Erik Huelsmann <eh...@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. Just
check it with following script:
[in test working copy]
$ echo foo >foo
$ svn add foo
$ svn ps prop value foo
$ svn ci -m "r1"
$ svn rm foo
$ echo foo >foo
$ svn add foo
$ svn ps prop "new value" foo
At this point check that .svn/prop-base/foo.svn-base exists, but
revert base doesn't.
I'd say more: prop-base always exists unless there are no props or
file isn't committed to repository. For committed entry props file
doesn't exist, unless there are no modifications.
--
Ivan Zhakov
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org