New feature: add a new policy for fixing clashes #810

iblislin · 2016-12-14T02:40:47Z

Hi,
I add an new command line option for drive clashes. Let user switch the policy between rename (the original) and trash. When I encounter the clashes, I always trash it then re-push again. Hope this also useful for other user. And the simple usage is documented in the commit message.

To be honest, I never wrote any line of golang code before I decide to add this feature. I just browsed the golang tutorial at the last Sunday afternoon. Please read my code carefully and comment. :)

odeke-em

Very minor changes requested, code looks good. Could we also document this in the README.md, if possible?

odeke-em · 2016-12-18T03:14:18Z

src/clashes.go

+type FixClashesMode uint8
+
+const (
+	FixClashesRename FixClashesMode = iota


Let's make this 1 + iota, so that when we can catch the
zero/uninitialized value, and reject it.

done @ 9d078d9

odeke-em · 2016-12-18T03:14:31Z

src/clashes.go

+
+func autoTrashClashes(g *Commands, clashes []*Change) error {
+	for _, c := range clashes {
+		// make c.Op() == OpDelete


Let's just update this comment to

// Let's coerce this change to a deletion

done @ f7a68ea

odeke-em · 2016-12-18T03:14:46Z

src/clashes.go

+	}
+
+	if g.opts.canPrompt() {
+		g.log.Logln("Some clashes found, trashes them ?")


Please replace
trashes them ?
with
trash them?

done @ 04dcf5e

odeke-em · 2016-12-18T03:16:38Z

Hello there @iblis17, thank you for the PR and welcome to drive!

This is awesome, your first Go code looks great!
I've made 3 comments for minor feedback otherwise we are in good shape.

iblislin · 2016-12-18T10:07:32Z

README.md updated @ 283066c

odeke-em · 2016-12-18T10:34:00Z

Dope! One last thing, let's squash all these commits into one and then I'll merge it in. Just in
case you need to

$ git rebase -i head~6

and when prompted for the message, let's set it to something like this

clashes: add trash and no-prompt to deal with clashes

Added trash and no-prompt to clash. The default is `rename` but
in some cases it is convenient to trash duplicates. This PR makes
that possible.

Thank you!

Added trash and no-prompt to clash. The default is `rename` but in some cases it is convenient to trash duplicates. This PR makes that possible. New cli option for `drive clashes`: $ drive clashes -fix -fix-mode trash

iblislin · 2016-12-18T11:18:47Z

Rebased !

odeke-em · 2016-12-18T12:07:19Z

Thank you, LGTM!

jpambrun · 2016-12-19T14:54:01Z

This change is confusing when using "drive clashes -fix -fix-mode trash [remote folder]" since is trashes both files. When doing so, I expected the newest file to be kept. Deleting both files certainly fixed the clash, but so would "rm -Rf" 😉.

iblislin · 2016-12-19T17:06:12Z

@jpambrun Trashing them all and re-pushing is just my habit. Feel free to rename it and add the feature you want.

Maybe I should notice user that it will trash both of new and old files on README.md

ref: odeke-em#810

jpambrun · 2016-12-19T17:51:47Z

What is the purpose of this? It does not make a lot of sense.

Especially since in my case, I had no local copy.

iblislin · 2016-12-20T06:11:20Z

Those are just different use cases. And I think there are no correct one or wrong one.
If this feature is not fitting your requirement. what about adding another function or option, contributing the code back, and make drive better!

If you really want that feature and do not want to write code, please open a feature request. I can push it into my TODO stack... Maybe I will make it at some Sunday afternoon.

jpambrun · 2016-12-20T12:57:35Z

I'm sorry if that came out a bit harsh; that was not the intent. I don't feel very strongly about this issue/feature and the proposed changes make it clear that it is a destructive operation.

Personally, I wouldn't make any destructive operations the default behavior for this type of application. I wanted to fix clashes and I issued "drive clashes -fix -fix-mode trash" on in a clean ~/gdrive folder thinking it would fix clashes based on the documentation.

Anyway, I appreciate your contribution.

iblislin · 2016-12-20T14:16:40Z

Personally, I wouldn't make any destructive operations the default behavior for this type of application. I wanted to fix clashes and I issued "drive clashes -fix -fix-mode trash" on in a clean ~/gdrive folder thinking it would fix clashes based on the documentation.

Hearing a different need is not a bad thing, and it's the source of innovation of open source. :)

I will try to add a new policy called trash-old (or something like that), if I get free time.

jpambrun · 2016-12-20T14:28:10Z

Having different needs is not the issue I was trying to raise. The issue is with the expectations that my data is not going to be lost by "fixing" clashes. Deleting them all is not what a typical user would call a "fix". So basically,yes, having different expectations when it comes to the potential lost of data is a bad thing. The better solution would be to call your fix-mode trash-all and the one I proposed trash. The is the safest, clearest way to include your feature.

…

On Tue, Dec 20, 2016 at 9:16 AM Iblis Lin ***@***.***> wrote: Personally, I wouldn't make any destructive operations the default behavior for this type of application. I wanted to fix clashes and I issued "drive clashes -fix -fix-mode trash" on in a clean ~/gdrive folder thinking it would fix clashes based on the documentation. Hearing a different need is not a bad thing, and it's the source of innovation of open source. :) I will try to add a new policy called trash-old (or something like that), if I get free time. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#810 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAr2MGzdbtP6vuLQf9F8fT_E2tlIkWCRks5rJ-NLgaJpZM4LMeGc> .

iblislin · 2016-12-20T14:39:40Z

ok, I got it. (~~I think I am quite used to fix them via C4~~

odeke-em requested changes Dec 18, 2016

View reviewed changes

clashes: add trash mode and no-prompt to deal with clashes

4178a6d

Added trash and no-prompt to clash. The default is `rename` but in some cases it is convenient to trash duplicates. This PR makes that possible. New cli option for `drive clashes`: $ drive clashes -fix -fix-mode trash

iblislin force-pushed the clashes-trash branch from 283066c to 4178a6d Compare December 18, 2016 11:18

odeke-em merged commit fa3d326 into odeke-em:master Dec 18, 2016

odeke-em added the enhancement label Dec 18, 2016

odeke-em added this to the v0.3.9 milestone Dec 18, 2016

iblislin added a commit to iblislin/drive that referenced this pull request Dec 19, 2016

README: more about -fix-mdoe trash

5aca832

ref: odeke-em#810

iblislin added a commit to iblislin/drive that referenced this pull request Dec 19, 2016

README: more about -fix-mode trash

f881512

ref: odeke-em#810

iblislin added a commit to iblislin/drive that referenced this pull request Dec 19, 2016

clashes: more note about -fix-mode trash

cdd274a

ref: odeke-em#810

iblislin added a commit to iblislin/drive that referenced this pull request Dec 19, 2016

clashes: more notes about -fix-mode trash

0c7ba89

ref: odeke-em#810

iblislin mentioned this pull request Dec 19, 2016

clashes: more notes about -fix-mode trash #815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New feature: add a new policy for fixing clashes #810

New feature: add a new policy for fixing clashes #810

iblislin commented Dec 14, 2016

odeke-em left a comment

odeke-em Dec 18, 2016

iblislin Dec 18, 2016

odeke-em Dec 18, 2016

iblislin Dec 18, 2016

odeke-em Dec 18, 2016

iblislin Dec 18, 2016

odeke-em commented Dec 18, 2016

iblislin commented Dec 18, 2016

odeke-em commented Dec 18, 2016

iblislin commented Dec 18, 2016

odeke-em commented Dec 18, 2016

jpambrun commented Dec 19, 2016

iblislin commented Dec 19, 2016

jpambrun commented Dec 19, 2016

iblislin commented Dec 20, 2016 •

edited

Loading

jpambrun commented Dec 20, 2016

iblislin commented Dec 20, 2016

jpambrun commented Dec 20, 2016 via email

iblislin commented Dec 20, 2016

New feature: add a new policy for fixing clashes #810

New feature: add a new policy for fixing clashes #810

Conversation

iblislin commented Dec 14, 2016

odeke-em left a comment

Choose a reason for hiding this comment

odeke-em Dec 18, 2016

Choose a reason for hiding this comment

iblislin Dec 18, 2016

Choose a reason for hiding this comment

odeke-em Dec 18, 2016

Choose a reason for hiding this comment

iblislin Dec 18, 2016

Choose a reason for hiding this comment

odeke-em Dec 18, 2016

Choose a reason for hiding this comment

iblislin Dec 18, 2016

Choose a reason for hiding this comment

odeke-em commented Dec 18, 2016

iblislin commented Dec 18, 2016

odeke-em commented Dec 18, 2016

iblislin commented Dec 18, 2016

odeke-em commented Dec 18, 2016

jpambrun commented Dec 19, 2016

iblislin commented Dec 19, 2016

jpambrun commented Dec 19, 2016

iblislin commented Dec 20, 2016 • edited Loading

jpambrun commented Dec 20, 2016

iblislin commented Dec 20, 2016

jpambrun commented Dec 20, 2016 via email

iblislin commented Dec 20, 2016

iblislin commented Dec 20, 2016 •

edited

Loading