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.

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

Copy a file to the same directory #10825

Merged
merged 6 commits into from
Nov 22, 2018
Merged

Copy a file to the same directory #10825

merged 6 commits into from
Nov 22, 2018

Conversation

greenido
Copy link

@greenido greenido commented Aug 23, 2018

  1. Added the logic to change the new file name to 'xxxx_1' (or 2,3,4, when there are already copies of the file).
  2. After the copy there was a need to reload the list of files in order to show the user the changes.

Fixes #9931

.gitignore Outdated Show resolved Hide resolved
.htaccess Outdated Show resolved Hide resolved
@MorrisJobke
Copy link
Member

@skjnldsv Let's make it 15 and then backport to stable14 😉

@greenido
Copy link
Author

The changes @skjnldsv suggested are in the last commit: 83bd2f5
Thx.

@MorrisJobke
Copy link
Member

@skjnldsv @jancborchardt @juliushaertl Mind to review this one?

@MorrisJobke
Copy link
Member

@dairaine @sunjam @lightweight Could you try this change for the feature you requested?

@MorrisJobke MorrisJobke changed the title Fixing issue 9931 - copy a file to the same directory Copy a file to the same directory Oct 2, 2018
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greenido nice! Could you adjust the naming to the spec at #9931 (comment) ?

  • filename (copy).txt
  • filename (another copy).txt
  • filename (3rd copy).txt
  • filename (4th copy).txt
  • etc

This is more obvious than just appending the number, which could also be a conflict or having uploaded the original file again.

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 2, 2018
@greenido
Copy link
Author

greenido commented Oct 2, 2018 via email

@greenido
Copy link
Author

greenido commented Oct 3, 2018 via email

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need some small changes!! Did not tested it yet, the code looks good 👍✨

apps/files/js/filelist.js Outdated Show resolved Hide resolved
apps/files/js/filelist.js Outdated Show resolved Hide resolved
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first copy does not need a number, just Filename (copy).txt :) Otherwise it seems good. 👍

@MorrisJobke
Copy link
Member

@jancborchardt @skjnldsv Mind to re-review?

@MorrisJobke MorrisJobke mentioned this pull request Nov 7, 2018
29 tasks
@MorrisJobke
Copy link
Member

@greenido As we are planning to release 15.0.0 Beta 2 today: I guess there is very little chance that you could sign-off the commits, right? It's fairly easy:

Let's do this for 16 and look into back porting this maybe later.

@greenido
Copy link
Author

greenido commented Nov 15, 2018 via email

@MorrisJobke
Copy link
Member

@greenido No need to hurry on this one. Beta 2 is out 🚀 but we will get it into the next release maybe ;)

Copy link
Author

@greenido greenido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed-off-by: Ido Green greenido@gmail.com

@kesselb
Copy link
Contributor

kesselb commented Nov 15, 2018

@greenido https://github.com/nextcloud/server/runs/32106619 follow the instructions there to fix the sign off

@greenido
Copy link
Author

greenido commented Nov 15, 2018 via email

@kesselb
Copy link
Contributor

kesselb commented Nov 15, 2018

Yes. I'm not sure but greenido/server is missing a lof ot changes form nextcloud/server. I would try to update my fork first (https://help.github.com/articles/syncing-a-fork/) and than try to rebase again (https://github.com/nextcloud/server/pull/10825/checks?check_run_id=32202446).

@greenido
Copy link
Author

greenido commented Nov 15, 2018 via email

@MorrisJobke
Copy link
Member

Just did... thank you for the heads up.

This looks like it went wrong 🙈 Sorry for the mess here. I pushed to your branch a new commit as well and this seems to not have relaxed the situation.

I guess here a heavy "git rebase --interactive" is needed with "edit" as option instead of "pick" to be able to modify the commit message (adding basically the sign-off message). See https://morrisjobke.de/2015/12/03/How-to-do-a-git-rebase/ for details about a rebase.

In your case it's best to checkout master. Do a pull from our master. Then you can do the above mentioned rebase and push again. In case of problems: I have the old version of the branch still locally and we can go for that if something bad happens.

@MorrisJobke
Copy link
Member

And this is how the sign-off looks like:

80f4c40

It's basically this line inside the commit message:

Signed-off-by: Morris Jobke <morris@example.org>

@greenido
Copy link
Author

greenido commented Nov 15, 2018

Hey @MorrisJobke

I followed the steps you mention but I don't know what to pick in the interactive merge...
Please see:

Using index info to reconstruct a base tree...
M	core/js/setupchecks.js
M	settings/Controller/CheckSetupController.php
Falling back to patching base and 3-way merge...
Auto-merging settings/Controller/CheckSetupController.php
CONFLICT (content): Merge conflict in settings/Controller/CheckSetupController.php
Auto-merging core/js/setupchecks.js
CONFLICT (content): Merge conflict in core/js/setupchecks.js
error: Failed to merge in the changes.
Patch failed at 0043 Adds a setup check for the memory limit

Can you fix these two in 'my' local master (or branch)?
Thanks!

@MorrisJobke
Copy link
Member

I just used the local commits from my side, rebased them and pushed them. They do not yet contain the sign-off message, but I can walk you through this.

git rebase --interactive def8af3e7e

This will open an editor with following content:

pick 0dad9c1397 fixing issue 9931 - copy a file to the same directory
pick 1d827da7e2 cleaning after comments on issue 10825
pick 8eab3e8cb1 Use the new naming per the comments on #9931
pick d025c92108 Per the last comment on having only (copy) for the first one
pick bdf3019e4e Translate name for "(copy)"

# Rebase def8af3e7e..bdf3019e4e onto def8af3e7e (5 commands)
#
# Commands:
# p, pick = use commit
# r, reword = use commit, but edit the commit message
# e, edit = use commit, but stop for amending
# s, squash = use commit, but meld into previous commit
# f, fixup = like "squash", but discard this commit's log message
# x, exec = run command (the rest of the line) using shell
# d, drop = remove commit
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.
#
# Note that empty commits are commented out

The first 4 lines in there are your commits, the fifth one is my commit. So you only need to edit the first 4 lines. Change there the pick into edit. This basically does a pick and gives you a prompt after each of those commits. Then you can run git commit --amend -s to add the sign-off message. Continue then to the next commit with git rebase --continue. Do there the same. Repeat this until it says:

Successfully rebased and updated refs/heads/fixing-issue-9931.

Force push your commits and you should be done. 👍

@greenido
Copy link
Author

I followed your steps - but for some reason that DCO still asking me to sign more commits.
Help! :)

@MorrisJobke
Copy link
Member

Uuuuhhh ... the git history looks a bit broken. It seems that there were non-fast-forward merges on your master branch and thus git thinks that there needs to be more merged into this one here. Also your commits do not contain the message. :/ I could add the message for you and fix this PR. In the future you could use git commit -s to let git add the message right while creating the commit and this then does not require this huge rebasing here anymore.

Ido Green and others added 5 commits November 19, 2018 13:56
Signed-off-by: Ido Green <greenido@gmail.com>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
More: #10825 (comment)

Signed-off-by: Ido Green <greenido@gmail.com>
More details here: #9931 (comment)

Signed-off-by: Ido Green <greenido@gmail.com>
Signed-off-by: Ido Green <greenido@gmail.com>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 19, 2018
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke
Copy link
Member

stable15 is branched off -> merge

@MorrisJobke MorrisJobke merged commit 68ad2ae into nextcloud:master Nov 22, 2018
@welcome
Copy link

welcome bot commented Nov 22, 2018

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22
Most developers hang out on IRC. So join #nextcloud-dev on Freenode for a chat!

@greenido
Copy link
Author

greenido commented Nov 22, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants