Usually darcs replace works gracefully even if the file has been
edited and edits contain new tokens.

However, with darcs 2, darcs replace fails on already darcs mv'd files.

Test script attached.

Attachments

• replace-in-moved.sh

Pekka: thanks! Please stick this in bugs/ and send a patch :-)

Test script as dpatch as requested...

Attachments

• added-bugs_replace_in_moved_sh-for-bug-864_.dpatch

Hmm... I got the following error, even though I'm up to date:

darcs: Cannot apply this patch bundle, since we're missing:
Thu May 15 23:50:20 BST 2008  Pekka Pessi <pekka.pessi@nokia.com>
  * Linking multiple times with libwww components with -static

  Libwww --libs are in wrong order for static linking.

How about just darcs sending the patch?

2008/5/19 Eric Kow <bugs@darcs.net>:
>  How about just darcs sending the patch?

Sorry, I was sending against my own repo and not darcs.net. Here is
patch with correct context.

Attachments

• added-bugs_replace_in_moved_sh-for-bug-864_.dpatch

I've taken an initial look at this issue (without looking at any
code), and here's what I've found out so far.

1) Replace --force will perform the replace in a moved file, but
in a not-moved file the replace will happen even without
--force.

About --force A Replace to recorded existing tokens must
introduce automatic reverse changes in the working copy to allow
the replace to be invertible. These changes my lump together
with unrecorded changes, but apart from that they should be
harmless and revertible. A Replace to unrecorded existing tokens
however is destructive and irreversible.

Therefore I think the --force flag is more appropriate for
unrecorded tokens than for recorded tokens, and the fix should
be to require --force also for not-moved files, and to change
the warning message to not only speaks about recorded tokens.

2) There seams to be something deeper behind this bug. Here is a
capture of a very strange session:

  ; darcs what
  move ./file1 ./file2
  hunk ./file2 5
  -bbbb
  +bb
  replace ./file2 [A-Za-z_0-9] bb bbbb
  ; darcs rev
  move ./file1 ./file2
  Shall I revert this change? (1/3)  [ynWvpxdaqjk], or ? for help: n
  hunk ./file2 5
  -bbbb
  +bb
  Shall I revert this change? (2/3)  [ynWsfvpxdaqjk], or ? for help: y
  Skipped revert of 1 patch.
  Do you really want to revert these changes? y
  Finished reverting.
  ; darcs what
  move ./file1 ./file2

Darcs has the dependency between the Hunk and the Replace right,
but the dialog is wrong. I shouldn't be able to revert the Hunk
without first reverting the Replace. If I do revert the Hunk
(which I did) darcs must also revert the Replace, which it did
with the misinforming message "Skipped revert of 1 patch.". The
expected dialog should ask about the Replace first, and if I
answer No to the Replace darcs should skip the revert question
for the Hunk.


I will now start to look at the code to see if I can find
something there. My email is out of order for another 24 hours
or so, so meanwhile I will check this issue in the bugtracker
for comments.

Ok, I now know why replace fails when the file is pendingly
moved. The replace command checks if the replace can be
performed cleanly in either working or pristine, and if it can
be cleanly applied in one of them (or the force flag is used)
the replace is performed. Otherwise a warning is issued. When
the file is moved but not yet recorded, the file does not exist
in pristine, so replace in pristine fails. The new token exists
in working, so replace in working also fails.

A twist here is that darcs allows for two pending moves like
this:

  file2 -> trick
  file1 -> file2

If the recorded file2 renamed to trick does not contain the new
token, the replace on the unrecorded file2 will now succeed
again without the --force flag.

The Replace command slurps recorded (pristine) and unrecorded,
but if I slurp pristine+pending instead of just pristine, the
test for this issue passes, and so does all the other tests in
the test suite.

  hunk ./src/Darcs/Commands/Replace.lhs 29
  -                    add_to_pending,
  +                    add_to_pending, slurp_pending,
  hunk ./src/Darcs/Commands/Replace.lhs 136
  -  (cur, work) <- slurp_recorded_and_unrecorded repository
  +  (_, work) <- slurp_recorded_and_unrecorded repository
  +  cur <- slurp_pending repository

This is how it was done before, although I don't know if it is
more correct..I don't really think we should go back to the old
behavior, since it can unintentionally destroy unrecorded
changes.

The pristine in Replace is only used to check if the replace can
be performed without force. Since the resulting replace patch
(with or without force) is applied to pending it is correct to
check if the replace can be performed against pending. Checking
it against pure pristine is incorrect, and can produce false
positives, as in the file move trick explained previously. So I
propose that this change resolves this issue, and will be
sending it as a patch.

As for the destroyed unrecorded changes I propose a change to
only perform the replace without --force if the replace succeeds
cleanly in *both* working and pending. This will make Replace
safe, and consistent with it's current documentation in the
manual.

The strange Revert dialog seams to be a totally different issue,
so I'll open a new bug for it.

Patches coming when my email starts to work tomorrow.

Unless I'm mistaken, this was resolved by the patch:

Sun Oct  5 00:57:19 BST 2008  Tommy Pettersson <ptp@lysator.liu.se>
  * resolve issue864: check non-force replace against pending
  The replace was checked against pure pristine, so the answer to if it could
  be applied to pending without force was sometimes wrong.

I guess our roundup-darcs integration must have been offline at the time

I confirm that the regression test passes.