You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@wandisco.com> on 2010/05/12 17:55:08 UTC
Commiting, tree conflicts and keep-local
The only thing keeping me from finally removing svn_wc_entry_t from
harvest_committables is keep_local. There was some discussion on IRC
a few days ago
http://colabti.org/irclogger/irclogger_log/svn-dev?date=2010-05-06#l44
but it really resolve things. It includes pointers to Neels patch to
make keep-local remove tree conflicts.
Tree conflicts generally prevent a commit, however if the conflict is
inside a directory that has been deleted with "svn rm --keep-local"
then the commit will be allowed. So
rm -rf repo wc
svnadmin create repo
svn mkdir -mm file://`pwd`/repo/A
svn mkdir -mm file://`pwd`/repo/A/B
svn co -r1 file://`pwd`/repo wc
svn cp file://`pwd`/repo/A wc/A/B
svn up wc # creates tree conflict for A/B
svn ci -mm wc # fails because of tree conflict
svn rm wc/foo # fails because of tree conflict
svn rm --keep-local wc/A # succeeds
svn ci -mm wc # succeeds
When A is rm'd the tree conflict is sort of still present, it shows up
in status, but it's all a bit dodgy. After the commit the unversioned
directory A/B still contains a .svn directory. If I revert, rather
than commit, then wc/A/B becomes unversioned.
In wc-metadata.sql there is a comment about working_node.keep_local
being temporary.
My immediate problem is how to replace entry->keep_local in
harvest_committables. Two options are:
1. XFAIL tree_conflicts 17.
2. Add svn_wc__db_temp_get_keep_local.
3. Use Neels' patch to make rm --keep-local/--force remove tree conflicts.
Any other ideas?
--
Philip
Re: Commiting, tree conflicts and keep-local
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Aug 25, 2010 at 05:00:09PM +0100, Julian Foad wrote:
> On Tue, 2010-08-24, Julian Foad wrote:
> [...]
> > What should happen?
> > -------------------
> >
> > I think the required changes are:
> >
> > * Commit should unconditionally bail out if there are any conflicts
> > inside a node being committed. No more testing the 'keep_local' flag at
> > this stage.
>
> I'm sure that's the behaviour we should have. The only thing I don't
> understand is why Neels changed harvest_committables() to ignore
> conflicts if 'keep_local' is set in r878027. That commit was about
> fixing some problems with keep_local, but it's not clear to me why this
> particular change was made. I assumed it was to preserve some 1.6.x
> behaviour, but I've just tested with a 1.6.x branch build and it doesn't
> allow the commit.
>
> I'm removing that condition, so the commit will fail if there are
> unresolved conflicts, even inside a directory scheduled for delete.
> This doesn't cause any test failures except tree_conflicts_tests 17
> (which is specifically about this behaviour) and it brings the commit
> code back into line with 1.6.x in which
> bail_on_tree_conflicted_children() was called unconditionally.
>
> I've committed this in 989189.
>
> If Neels or anyone can confirm this diagnosis, that would be great.
I guess that auto-resolving tree conflicts inside a locally deleted
subtree was intended as a user convenience.
But requiring an explicit 'svn resolve' invocation to resolve the conflict
before commit is in line with the general behaviour of Subversion, so I think
your change (reverting Neels changes) is sound.
Stefan
Re: Commiting, tree conflicts and keep-local
Posted by Julian Foad <ju...@wandisco.com>.
I can now clarify the desired behaviour, and explain why "--keep-local"
shouldn't make any difference. Resolving a conflict is supposed to be a
two step process for the user:
1. Choose the desired WC state - e.g. use "svn delete" or something
else.
2. Tell Subversion it's resolved - use "svn resolved".
Of course there's (supposed to be) a short-cut to most of the simple
resolutions that does both steps with one command - "svn resolve
--accept=XXX".
The point is that "svn delete" shouldn't automatically resolve any
conflict, and "svn commit" should not allow a commit that contains a
conflict, so there's no reason to think "svn delete" followed by "svn
commit" should succeed if there was a conflict.
On Tue, 2010-08-24, Julian Foad wrote:
[...]
> What should happen?
> -------------------
>
> I think the required changes are:
>
> * Commit should unconditionally bail out if there are any conflicts
> inside a node being committed. No more testing the 'keep_local' flag at
> this stage.
I'm sure that's the behaviour we should have. The only thing I don't
understand is why Neels changed harvest_committables() to ignore
conflicts if 'keep_local' is set in r878027. That commit was about
fixing some problems with keep_local, but it's not clear to me why this
particular change was made. I assumed it was to preserve some 1.6.x
behaviour, but I've just tested with a 1.6.x branch build and it doesn't
allow the commit.
I'm removing that condition, so the commit will fail if there are
unresolved conflicts, even inside a directory scheduled for delete.
This doesn't cause any test failures except tree_conflicts_tests 17
(which is specifically about this behaviour) and it brings the commit
code back into line with 1.6.x in which
bail_on_tree_conflicted_children() was called unconditionally.
I've committed this in 989189.
If Neels or anyone can confirm this diagnosis, that would be great.
> * Either the regression test should call "svn resolved" before
> attempting the commit, or the "svn delete" that it performs should clear
> the conflicts from inside the directory that it deleted.
There is no special behaviour that needs to be tested here, so the whole
test should go away. (If anything needs testing, it is that "svn delete
--keep-local" works the same as "svn delete" except that it keeps the
local nodes on disk. Not a tree conflict test.)
So I've deleted these two tests:
17 --keep-local del on dir with TCs inside
19 XFAIL --keep-local del on tree-conflicted targets
which were added in r878027 and r878028 respectively.
The one that wasn't already XFail was failing in single-DB mode, which
is what got me started looking at this.
I'm also reconsidering the validity of these two tests:
18 XFAIL --force del on dir with TCs inside
20 XFAIL --force del on tree-conflicted targets
which were added in r878028 on the basis that "delete --force" followed
by "commit" should behave the same way. I may have agreed with that at
the time. But why should it? Maybe "delete --force" should clear tree
conflicts in the directory being deleted, I don't know, but it certainly
shouldn't leave a special intermediate state of "disarmed tree conflict"
that "commit" treats as special, which is what these tests are currently
expecting.
> * None of this behaviour should depend on whether the user wants an
> unversioned copy of the node to be kept on disk.
>
> Neels, in the IRC log below it looks like you had a patch for making
> "svn delete" clear tree conflicts when "--force" or "--keep-local" is
> given. I don't see why we'd want "--keep-local" to behave specially,
> but with "--force", sure, that should clear conflicts. Do you still
Actually I don't know that we definitely want "delete --force" to clear
tree conflicts.
> have the patch? (The paste linked from IRC has expired.)
>
> <http://colabti.org/irclogger/irclogger_log/svn-dev?date=2010-05-06#l44>
- Julian
Re: Commiting, tree conflicts and keep-local
Posted by Julian Foad <ju...@wandisco.com>.
I'm looking at the tree_conflict_tests.py 17 failure in single-DB.
FAIL: tree_conflict_tests.py 17: --keep-local del on dir with TCs
inside
Neels, you might be able to shed some light on this.
What's the test for?
--------------------
The test says: if you have a directory with tree conflicts inside it,
you should be able to delete that directory (with the "--keep-local"
flag) and then commit the result, without having to resolve the tree
conflicts as an explicit step.
I'm not sure if that is really desired behaviour any more. AFAIK, the
intention is that the user must always resolve conflicts before
committing.
But the steps it goes through are odd. It first schedules the dir for
deletion, but leaving tree conflicts visible inside it, like Philip
observed back in May (see quoted msg below). Then it tries to commit
that delete-a-directory-with-conflicts-inside, which definitely seems
wrong, even if it "worked" in 1.6. And why is the "--keep-local"
command-line flag involved? I see no logical reason why that should
make a difference.
I wonder if the test is just preserving historical but unwanted
behaviour.
What happens in multi-DB mode?
------------------------------
The "svn delete --keep-local" marks the directory as to-be-deleted and
sets the "keep_local" flag in the DB, to tell the post-commit processing
not to remove the directory after committing its deletion. So far so
good. But it leaves the directory's children marked as conflicted,
which shows up in "status":
[[[
$ svn status
D 3 3 jrandom A/C
? C A/C/file
> local edit, incoming delete upon update
? C A/C/dir
> local edit, incoming delete upon update
]]]
At commit time, harvest_committables() says:
if (! keep_local)
SVN_ERR(bail_on_tree_conflicted_children(..., local_abspath,
db_kind, ...));
Thus it doesn't check for conflicted children, and the commit goes
ahead. In commit post-processing, the directory is left on disk but its
metadata is all removed so it no longer has any conflicts recorded in
it.
What happens in single-DB mode?
-------------------------------
The same, except that no "keep_local" flag is stored in the DB, and when
harvest_committables() tries to read it back it sees FALSE. Thus
harvest_committables() DOES call bail_on_tree_conflicted_children() and
the commit fails because there are tree-conflicted children.
What should happen?
-------------------
I think the required changes are:
* Commit should unconditionally bail out if there are any conflicts
inside a node being committed. No more testing the 'keep_local' flag at
this stage.
* Either the regression test should call "svn resolved" before
attempting the commit, or the "svn delete" that it performs should clear
the conflicts from inside the directory that it deleted.
* None of this behaviour should depend on whether the user wants an
unversioned copy of the node to be kept on disk.
Neels, in the IRC log below it looks like you had a patch for making
"svn delete" clear tree conflicts when "--force" or "--keep-local" is
given. I don't see why we'd want "--keep-local" to behave specially,
but with "--force", sure, that should clear conflicts. Do you still
have the patch? (The paste linked from IRC has expired.)
<http://colabti.org/irclogger/irclogger_log/svn-dev?date=2010-05-06#l44>
- Julian
On Wed, 2010-05-12, Greg Stein wrote:
> Temp function. It can be used in entries.c, too.
>
> It'll only last until single-db.
>
> Not blasting conflict info is good, in case of reverts.
>
> On May 12, 2010 2:17 PM, "Philip Martin" <ph...@wandisco.com> wrote:
>
> The only thing keeping me from finally removing svn_wc_entry_t from
> harvest_committables is keep_local. There was some discussion on IRC
> a few days ago
> http://colabti.org/irclogger/irclogger_log/svn-dev?date=2010-05-06#l44
> but it really resolve things. It includes pointers to Neels patch to
> make keep-local remove tree conflicts.
>
> Tree conflicts generally prevent a commit, however if the conflict is
> inside a directory that has been deleted with "svn rm --keep-local"
> then the commit will be allowed. So
>
> rm -rf repo wc
> svnadmin create repo
> svn mkdir -mm file://`pwd`/repo/A
> svn mkdir -mm file://`pwd`/repo/A/B
> svn co -r1 file://`pwd`/repo wc
> svn cp file://`pwd`/repo/A wc/A/B
> svn up wc # creates tree conflict for A/B
> svn ci -mm wc # fails because of tree conflict
> svn rm wc/foo # fails because of tree conflict
> svn rm --keep-local wc/A # succeeds
> svn ci -mm wc # succeeds
>
> When A is rm'd the tree conflict is sort of still present, it shows up
> in status, but it's all a bit dodgy. After the commit the unversioned
> directory A/B still contains a .svn directory. If I revert, rather
> than commit, then wc/A/B becomes unversioned.
>
> In wc-metadata.sql there is a comment about working_node.keep_local
> being temporary.
>
> My immediate problem is how to replace entry->keep_local in
> harvest_committables. Two options are:
>
> 1. XFAIL tree_conflicts 17.
> 2. Add svn_wc__db_temp_get_keep_local.
> 3. Use Neels' patch to make rm --keep-local/--force remove tree conflicts.
>
> Any other ideas?
>
> --
> Philip
Re: Commiting, tree conflicts and keep-local
Posted by Greg Stein <gs...@gmail.com>.
Temp function. It can be used in entries.c, too.
It'll only last until single-db.
Not blasting conflict info is good, in case of reverts.
On May 12, 2010 2:17 PM, "Philip Martin" <ph...@wandisco.com> wrote:
The only thing keeping me from finally removing svn_wc_entry_t from
harvest_committables is keep_local. There was some discussion on IRC
a few days ago
http://colabti.org/irclogger/irclogger_log/svn-dev?date=2010-05-06#l44
but it really resolve things. It includes pointers to Neels patch to
make keep-local remove tree conflicts.
Tree conflicts generally prevent a commit, however if the conflict is
inside a directory that has been deleted with "svn rm --keep-local"
then the commit will be allowed. So
rm -rf repo wc
svnadmin create repo
svn mkdir -mm file://`pwd`/repo/A
svn mkdir -mm file://`pwd`/repo/A/B
svn co -r1 file://`pwd`/repo wc
svn cp file://`pwd`/repo/A wc/A/B
svn up wc # creates tree conflict for A/B
svn ci -mm wc # fails because of tree conflict
svn rm wc/foo # fails because of tree conflict
svn rm --keep-local wc/A # succeeds
svn ci -mm wc # succeeds
When A is rm'd the tree conflict is sort of still present, it shows up
in status, but it's all a bit dodgy. After the commit the unversioned
directory A/B still contains a .svn directory. If I revert, rather
than commit, then wc/A/B becomes unversioned.
In wc-metadata.sql there is a comment about working_node.keep_local
being temporary.
My immediate problem is how to replace entry->keep_local in
harvest_committables. Two options are:
1. XFAIL tree_conflicts 17.
2. Add svn_wc__db_temp_get_keep_local.
3. Use Neels' patch to make rm --keep-local/--force remove tree conflicts.
Any other ideas?
--
Philip