uncommit/revert sometimes deletes, sometimes unversions

[Kirill Müller]

During work on rebase-interactive functionality, I have discovered the following strange behavior. Suppose we add several files, one per commit. When changing the order of these commits (e.g., using a sequence of merge commands) and issuing "uncommit" commands afterwards, some files appear to be deleted by the "revert" command. I would expect the files to remain as "unknown" and to get deleted only by a "clean-tree" command.

I have attached a bash test, tested against both bzr 2.3.4 from Ubuntu and bzr trunk -r6145 and -r6217. The test is minimal in the sense that I was unable to find a smaller example.

The output of bzr --version is found below. I am not familiar enough with the bzrlib API to easily fabricate a proper test case, but I'd be happy to get a pointer to the converted code.

Bazaar (bzr) 2.3.4
  Python interpreter: /usr/bin/python 2.7.1
  Python standard library: /usr/lib/python2.7
  Platform: Linux-2.6.38-11-generic-x86_64-with-Ubuntu-11.04-natty
  bzrlib: /usr/lib/python2.7/dist-packages/bzrlib

Bazaar (bzr) 2.5.0dev2
  from bzr checkout /home/muelleki/bzr/bzr
    revision: 6145
    revid: <email address hidden>
    branch nick: bzr
  Python interpreter: /usr/bin/python 2.7.1
  Python standard library: /usr/lib/python2.7
  Platform: Linux-2.6.38-11-generic-x86_64-with-Ubuntu-11.04-natty

Bazaar (bzr) 2.5.0dev3
  from bzr checkout /home/muelleki/bzr/bzr
    revision: 6217
    revid: <email address hidden>
    branch nick: bzr
  Python interpreter: /usr/bin/python 2.7.1
  Python standard library: /usr/lib/python2.7
  Platform: Linux-2.6.38-11-generic-x86_64-with-Ubuntu-11.04-natty

[Martin Pool (mbp)]

thanks,

possibly the easiest way to make a test for this is to turn your
script into a shell-like test: see
http://doc.bazaar.canonical.com/developers/testing.html#shell-like-tests

If you would like to contribute a branch that adds even just a failing
test that would be very welcome.

[Kirill Müller (krlmlr)]

Martin,

thank you. In which module should this test live?

[Martin Pool (mbp)]

So where it goes depends a bit on where we think the bug is, but it
could arguably be in tests/blackbox/test_revert, and to some extent
these things are a bit arbitrary.

But first: is this a bug: what you're seeing is basically that after
uncommitting, you have a tree with a pending merge that is adding the
file 'b'. If you revert, b is deleted.

You're correct that this is in some ways inconsistent: add b directly
yourself, then revert, and 'b' remains as unversioned. But add b via
a merge, and revert removes it.

Pragmatically this is probably ok in that if you just added the file,
it may be your only copy, whereas if you just merged it, you can
probably merge again.

This is connected to, or a reflection of, the fact that merging
normally does delete removed files rather than leaving them. We could
make that configurable, which would let you perhaps get the desired
behaviour during rebase-interactive.

[Kirill Müller (krlmlr)]

Well, the story goes further. Uncommitting the next commit leaves c "unversioned", just as expected. I haven't added this to the test, since this is the expected behavior. The extended test (attached) covers this.

Any thoughts?

[Martin Pool (mbp)]

I think the problem is still basically what I said, which is that revert handles added-by-merge files differently, and that this ought to be at least made configurable. If you want to try doing that I'd be happy to review it. I would love to see an interactive rebase added.

[Kirill Müller (krlmlr)]

But in my example, there are two merges -- the one that adds c, and the other that adds b. When uncommiting and reverting, b gets deleted, c gets unversioned. What makes bzr behave differently in these two cases?

I could contribute by testing various corner cases, but I don't know enough about bzrlib internals to be able to properly fix this issue.

[Martin Pool (mbp)]

I think the next sensible step on this is to actually convert your
script into a test and then work out why revert is being inconsistent
between the two things. I'll try to have a look at that soon but it's
not quite at the top of my listn.