Ticket #2276 (closed defect: fixed)

Opened 6 years ago

Last modified 6 years ago

copy/move: wrong directory update with the same name

Reported by: lly Owned by: slavazanko
Priority: major Milestone: 4.7.4
Component: mc-core Version: master
Keywords: Cc: zaytsev
Blocked By: Blocking:
Branch state: Votes for changeset: commited-master

Description

Steps to reproduce:
1) Left panel /home/ - has directory aaa1 with some files
2) Right panel /u00/ - also has directory aaa1 with some files
3) Try to copy(F5) directory aaa1 from Left panel to Right
4) Result - /u00/aaa1/aaa1 instead of updated content of /u00/aaa1

i.e. directory erroneously copies to one level deeper, move(F6) is also affected. This behavior absent in 4.7.0.6

Change History

comment:1 Changed 6 years ago by zaytsev

  • Cc zaytsev added
  • Version changed from 4.7.3 to master

This problem does not exist on 4.7.0.7. I think it was introduced in #1907.

comment:2 Changed 6 years ago by x905

confirm, mc 4.7.3-26-g848c2ad bug still there

comment:3 Changed 6 years ago by andrew_b

Yes, this bug was introduced in #1907.

The simplest possible solution: append file name only, not directory name.

comment:4 follow-up: ↓ 5 Changed 6 years ago by zaytsev

Why not just check whether the directory already exists on the target panel or not and if it does, refrain from appending the directory name?

comment:5 in reply to: ↑ 4 Changed 6 years ago by ossi

Replying to zaytsev:

Why not just check [...]

that sounds wrong. the ui should behave consistently, regardless of the contents of the target panel.

comment:6 Changed 6 years ago by zaytsev

It will behave consistently in all cases apart from this very special case, in which I would still qualify my suggested behavior as consistent. The solution to not to append directory names at all is IMO more inconsistent than that.

An alternative solution to check at the copy time does not sound right to me. What if the user actually DID want to copy this directory inside the directory of the same name?

comment:7 Changed 6 years ago by ossi

look at the posix rename api. it is absolutely unambiguous. the problem is, that it can represent only exactly one rename at a time. that's why the command line tools change their behavior depending on whether the target is an existing directory (which i don't like at all). but that still doesn't happen in the "ui".

comment:8 Changed 6 years ago by zaytsev

In my understanding the consistent behavior is the one that is consistent with the command line tools everybody is used to. What other UI you are talking about with which mc should be consistent rather than with cp etc. I don't understand.

comment:9 Changed 6 years ago by ossi

you know what the "ui" stands for, and the that a command line is also a ui, right?

anyway, i'm not sure any more what you were actually suggesting, as in fact you didn't say anywhere at which level you want to do the special-casing.

comment:10 follow-up: ↓ 12 Changed 6 years ago by x905

please fix this bug - its bad by design
i cant use mc with that

comment:11 Changed 6 years ago by zaytsev

x905, did you notice that people are discussing the way to solve the issue?

ossi, I suggested to make it work the way cp works right now. That is not to append the directory name by default whenever the target directory already exists. I am not sure what exactly you found inconsistent in this behavior.

comment:12 in reply to: ↑ 10 Changed 6 years ago by andrew_b

Replying to x905:

please fix this bug - its bad by design
i cant use mc with that

If you build mc youself from git, temporary revert the 9b5a8dec333a3d9724623c83cc0e60a121b5cc76 commit.

comment:13 Changed 6 years ago by andrew_b

To fix this bug and restore the status quo I propose revert the 9b5a8dec333a3d9724623c83cc0e60a121b5cc76 commit and reopen #1907.

comment:14 Changed 6 years ago by ossi

zaytsev, that added zero new information. :)
the question is whether you want to have the different appending behavior already in the ui or somewhere at the vfs level (the latter would be in line with cp/mv behavior).

comment:15 follow-up: ↓ 18 Changed 6 years ago by zaytsev

Hmmm... what's the point? I don't care in which ticket it will be fixed. Is it gonna be easier to backport?

All we need is to agree on a solution. Does anyone has any objections to my suggestion?

ossi: the vfs level is already in line with cp/mv behavior. It is just that #1907 introduced a new ui feature fro F5/F6. Now the name of the file or directory is automatically appended to the To: field, so in case if one wants to change it he don't have to retype, but just edit (this is what many tc guys were missing as a substitute for click and rename).

However, Ilia didn't take into account that if the target directory already exists, this feature will make mc copy the directory to a subdirectory in line with cp rules. So I suggest to not to append it automatically in this case, because its not equivalent to what this command is supposed to do.

Can you make yourself clear? What exactly you don't like and how do you want it to be fixed instead? Your comments are just confusing everybody.

comment:16 follow-up: ↓ 17 Changed 6 years ago by zaytsev

To make it even MORE clear: this appending was assumed to be equivalent to the respective cp command, which turned out not to be the case for this edge case:

cp file /folder == cp file /folder/file, but
cp folder1 folder2 != cp folder1 folder2/folder1

comment:17 in reply to: ↑ 16 Changed 6 years ago by andrew_b

Replying to zaytsev:

cp folder1 folder2 != cp folder1 folder2/folder1

The result of
cp -R folder1 folder2
is folder2/folder1

comment:18 in reply to: ↑ 15 Changed 6 years ago by andrew_b

Replying to zaytsev:

Hmmm... what's the point?

We can fix this bug immediately. As a result we will not have the bug but we will have the unimplemented enhancement. We can discuss the correct way how to implement that in proper ticket.

comment:19 Changed 6 years ago by zaytsev

As you wish, I don't care. I don't see the point anyway, because you not gonna release 4.7.4 just for this, are you? Those who are really suffering will revert the commit and rebuild from source anyway.

comment:20 Changed 6 years ago by x905

yes, i see discussion ossi vs zaytsev )
i just want a solution, and i try it (brunch)
but i want old behavior back as default

comment:21 Changed 6 years ago by x905

as for ticket #1907 - the solution may be a hot key to copy selected name to clipboard (is it exits ? i want it too), then in f5/f6 dialog just press "end" and paste name to edit

comment:22 Changed 6 years ago by slavazanko

  • Owner set to slavazanko
  • Status changed from new to accepted

comment:23 Changed 6 years ago by slavazanko

  • Keywords stable-candidate added
  • severity changed from no branch to on review
  • Milestone changed from 4.7 to 4.7.4

created branch 2276_cpmv_wrong_dest_dir:

Review, please.

comment:24 Changed 6 years ago by slavazanko

  • Keywords stable-candidate removed

Well, this is really no right way to fix it. #1907 have broken thinks. Need to revert patch 9b5a8dec333a3d9724623c83cc0e60a121b5cc76 and reopen #1907.

comment:25 Changed 6 years ago by slavazanko

comment:26 Changed 6 years ago by andrew_b

  • Votes for changeset set to andrew_b

comment:27 Changed 6 years ago by angel_il

  • Votes for changeset changed from andrew_b to andrew_b angel_il
  • severity changed from on review to approved

comment:28 Changed 6 years ago by slavazanko

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b angel_il to commited-master
  • Resolution set to fixed
  • severity changed from approved to merged

comment:29 Changed 6 years ago by slavazanko

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.