Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

copy/move: wrong directory update with the same name #2276

Closed
mc-butler opened this issue Jul 11, 2010 · 29 comments
Closed

copy/move: wrong directory update with the same name #2276

mc-butler opened this issue Jul 11, 2010 · 29 comments
Assignees
@slavaz
Labels
area: core Issues not related to a specific subsystem prio: medium Has the potential to affect progress
Milestone
4.7.4

Comments

@mc-butler
Copy link

Important

This issue was migrated from Trac:

Origin https://midnight-commander.org/ticket/2276
Reporter lly (lly.dev@….com)
Mentions zaytsev (@zyv)

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
@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jul 11, 2010 at 14:49 UTC (comment 1)

  • Cc set to zaytsev
  • 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.

@mc-butler
Copy link
Author

Changed by x905 on Jul 11, 2010 at 15:05 UTC (comment 2)

confirm, mc 4.7.3-26-g848c2ad bug still there

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jul 12, 2010 at 12:10 UTC (comment 3)

Yes, this bug was introduced in #1907.

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

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jul 12, 2010 at 12:22 UTC (comment 4)

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?

@mc-butler
Copy link
Author

Changed by ossi (@ossilator) on Jul 12, 2010 at 15:18 UTC (comment 4.5)

Replying to zaytsev:

Why not just check [...]

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

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jul 12, 2010 at 17:28 UTC (comment 6)

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?

@mc-butler
Copy link
Author

Changed by ossi (@ossilator) on Jul 12, 2010 at 18:05 UTC (comment 7)

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".

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jul 13, 2010 at 8:40 UTC (comment 8)

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.

@mc-butler
Copy link
Author

Changed by ossi (@ossilator) on Jul 13, 2010 at 11:53 UTC (comment 9)

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.

@mc-butler
Copy link
Author

Changed by x905 on Jul 13, 2010 at 11:59 UTC (comment 10)

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

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jul 13, 2010 at 12:10 UTC (comment 11)

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.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jul 13, 2010 at 12:26 UTC (comment 10.12)

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 [9b5a8de] commit.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jul 13, 2010 at 12:29 UTC (comment 13)

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

@mc-butler
Copy link
Author

Changed by ossi (@ossilator) on Jul 13, 2010 at 12:33 UTC (comment 14)

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).

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jul 13, 2010 at 12:40 UTC (comment 15)

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.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jul 13, 2010 at 12:42 UTC (comment 16)

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

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jul 13, 2010 at 12:49 UTC (comment 16.17)

Replying to zaytsev:

cp folder1 folder2 != cp folder1 folder2/folder1

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

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jul 13, 2010 at 13:19 UTC (comment 15.18)

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.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jul 13, 2010 at 13:26 UTC (comment 19)

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.

@mc-butler
Copy link
Author

Changed by x905 on Jul 13, 2010 at 16:01 UTC (comment 20)

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

@mc-butler
Copy link
Author

Changed by x905 on Jul 13, 2010 at 16:17 UTC (comment 21)

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

@mc-butler
Copy link
Author

Changed by slavazanko (@slavaz) on Jul 14, 2010 at 9:52 UTC (comment 22)

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

@mc-butler
Copy link
Author

Changed by slavazanko (@slavaz) on Jul 14, 2010 at 9:56 UTC (comment 23)

  • Milestone changed from 4.7 to 4.7.4
  • Severity changed from no branch to on review
  • Keywords set to stable-candidate

created branch 2276_cpmv_wrong_dest_dir:

  • [ae914737419e5f3221721a2554ed69deb1e47069]

Review, please.

@mc-butler
Copy link
Author

Changed by slavazanko (@slavaz) on Jul 14, 2010 at 10:30 UTC (comment 24)

  • Keywords stable-candidate deleted

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

@mc-butler
Copy link
Author

Changed by slavazanko (@slavaz) on Jul 14, 2010 at 10:33 UTC (comment 25)

New changeset for branch:

  • [91b430758a98da4287c2d1d54249637efecbe13d] (reverting [9b5a8de])

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jul 14, 2010 at 10:34 UTC (comment 26)

  • Votes set to andrew_b

@mc-butler
Copy link
Author

Changed by angel_il (@ilia-maslakov) on Jul 15, 2010 at 5:17 UTC (comment 27)

  • Votes changed from andrew_b to andrew_b angel_il
  • Severity changed from on review to approved

@mc-butler
Copy link
Author

Changed by slavazanko (@slavaz) on Jul 15, 2010 at 10:50 UTC (comment 28)

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

merge [cb85a8b]

@mc-butler
Copy link
Author

Changed by slavazanko (@slavaz) on Jul 15, 2010 at 10:50 UTC (comment 29)

  • Status changed from testing to closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@slavaz slavaz

Labels
area: core Issues not related to a specific subsystem prio: medium Has the potential to affect progress
Milestone
4.7.4
Development

No branches or pull requests
2 participants
@slavaz @mc-butler