Add support for file marking #18

leonklingele · 2017-01-08T14:18:49Z

This adds support to mark files & folders via the space key.
This is useful if you want to use godu to remove large files:

$ godu . | xargs rm

codecov-io · 2017-01-08T14:59:04Z

Current coverage is 100% (diff: 100%)

Merging #18 into master will not change coverage

@@           master   #18   diff @@
===================================
  Files           6     7     +1   
  Lines         153   207    +54   
  Methods         0     0          
  Messages        0     0          
  Branches        0     0          
===================================
+ Hits          153   207    +54   
  Misses          0     0          
  Partials        0     0

Powered by Codecov. Last update bb67644...00210c7

viktomas

Hi @leonklingele, Thanks so much for the PR! I left few comments. I'd like to ask you to consider storing the marked files in State. Benefits being:

The result is going to be ready immediately
Command should be extremely easy to test

Please have a think about it. I still appreciate the PR and if you think that the IsMarked information should still stay in File let's have a chat about it.

In addition I haven't had a chance to think about absolute paths (which would be the best as output of marked files). Whilst you were changing the code did you have any ideas?

viktomas · 2017-01-08T20:24:31Z

core/file_walker.go

+		wg.Wait()
+		close(c)
+	}()
+	go getMarkedRecursive("", &root, c, &wg)


I really like your parallel code. And I've learnt that you can use for .. range on a channel just from reading your PR. I wonder if parallelism here increases the complexity of GetMarked and will make it harder to test? Have you tried the function running in one thread?

Why do you think this will make it harder to test?
I verified that the binary resulting from go install -race . does not report any issues.

I meant unit tests. When I run core.StartProcessing as a separate go routine it made the testing much harder (https://github.com/viktomas/godu/blob/master/core/processor_test.go) Certain test cases don't even expect anything and they succeed when no deadlock happens (which is not optimal IMHO).

But that might as well be just me being new to golang. You might find it as easy to unit test go routines as normal functions..

viktomas · 2017-01-08T20:25:51Z

ignore.go

@@ -2,7 +2,7 @@ package main

 import (
 	"bufio"
-	"fmt"
+	"log"


What is the benefit compared to fmt?

log.Print* logs to stderr by default whereas the fmt.Print* commands log to stdout.
This change is required so the output of godu can be piped to e.g. xargs:

# This will try to remove a file named 'godu will walk through..' if we log to stdout $ godu . | xargs rm

Understand. Thanks for explanation.

viktomas · 2017-01-08T20:33:04Z

core/file_walker.go

+
+func getMarkedRecursive(parent string, file *File, c chan<- string, wg *sync.WaitGroup) {
+	defer wg.Done()
+	path := "."


Why is dot a special case? I don't think you should ever prepend the / in path = fmt.Sprintf("%s/%s", parent, file.Name).

Do you have any idea how to build the filepaths (ideally absolute)? I mentioned that I haven't come up with solution yet in #14 (comment)

I was pretty unhappy with this file-path detection as well. The logic needs to be updated, sorry.
There seems to be a bug if we scan a directory other that the current $PWD.

I need to think about this as well. The issues related to the path not being easily accessible are beginning to stack up (#19)

@leonklingele Please have a look if you could use or somehow change core.State.Path() for marking. Path() was originally designed for status line on top of the screen. Ideally we could use it for both.

viktomas · 2017-01-08T20:39:19Z

core/command.go

+
+func (m Mark) Execute(oldState State) (State, error) {
+	newState := copyState(oldState)
+	newState.Folder.Files[newState.Selected].ToggleIsMarked()


The idea here was (And I'm to blame for not stating it explicitly) That everything related to user interaction would be stored in state. I'd prefer to put a marked map[*File]string into the state and every Execution of Mark command would add (or remove) the File from the map.

It would be much easier to test. And you would have result ready straight as you finish the interactive mode. What do you think @leonklingele?

Valid point. I will change that and also add new code which uses different color for a marked file's line. Update coming tomorrow.

Thanks @leonklingele! And please update README with the new functionality 😄

leonklingele · 2017-01-09T22:55:49Z

Updated.

leonklingele · 2017-01-10T02:03:08Z

Please let me know what you think about adc2d7b, where a file's directory is stored as a pointer to another file (instead of a string of the directory's name)

viktomas · 2017-01-10T02:07:04Z

@leonklingele: I'm just in the middle of reviewing your PR, please hang on :)

viktomas

I Can see how much effort you spent on this PR @leonklingele. I installed it on my laptop and the functionality looks awesome! I'm not too fussed about the tcell rendering but the core is very important to me. Hope you don't think that I'm a nazi that has to always have things my way. Let me know if you disagree with something and we'll have a chat.

viktomas · 2017-01-10T02:10:03Z

core/command.go

+}
+
+type FileMarker struct {
+	Marked []*File


You can use map here, I was earlier suggesting map[*File]string where the string would be path computed from State.ancestors, but if you want to use it only as a set, you can use map[*File]struct{}.

Than you can add new *File to the map by Makred[newMarkedFile] = struct{} and figure out if file is marked by _, isMarked = Marked[fileToCheck]

Then we might only have a map instead of new type, not sure.

I'm not a fan of using maps with arbitrary values, i.e. where we only need to key.
But in this case it will greatly simplify the code. I've decided to use a map[*File]bool because we should dynamically generate a file's path. Updated MR.

viktomas · 2017-01-10T02:11:55Z

core/command.go


 // State represents system configuration after processing user input
 type State struct {
 	ancestors ancestors
 	Folder    *File
 	Selected  int
 	history   map[*File]int // history of all selected postions
+	*FileMarker


The state should be immutable and we'll always generate new one by defensively copying the old state. Thanks to that we don't have to worry about concurrency, because the only thing that is generating the new state is core.StartProcessing which transforms command into a new state.

FileMarker was replaced with a map.

viktomas · 2017-01-10T02:14:13Z

interactive/reporter.go

+		var marking string
+		isMarked := marker.IsMarked(file)
+		if marker.IsMarked(file) {
+			marking = "*"


This now fiddles a bit with the 5 char formatting of fileSize introduced in #22 can we add the marking at the end?

I prefer to have it at the very front. Otherwise – having lots of long file names and a non-color terminal – you will not see if a file is marked or not. I'll check on how to fix the jumping.

viktomas · 2017-01-10T02:18:14Z

godu.go

+	printMarked(lastState)
+}
+
+func printMarked(state *core.State) {


could you please move this to a separate file in core and write a test for it? It should be easily testable if you change the signature to printMarked(state *core.State, writer io.Writer) Example of such a test is in older godu version (https://github.com/viktomas/godu/blob/a7bb16532ebe4e303ed6f4b0845774c7b90b64f2/interactive/interactive_test.go)

viktomas · 2017-01-10T02:20:55Z

core/file_walker.go

 	Size  int64
-	IsDir bool
+	IsDir bool // TODO(leon): IsDir() => len(f.Files) > 0


Because we are calling core.PruneTree and removing files/folders that are smaller then certain limit, folder without any files in Files property can exist (https://github.com/viktomas/godu/blob/master/core/processor.go#L9)

This is exactly how I was thinking originally! 😃

Sorry, this was not intended to be checked in. You're right, it doesn't make sense to have that function.

viktomas · 2017-01-10T02:24:30Z

core/file_walker.go

 	"os"
 	"path/filepath"
 )

 type File struct {
 	Name  string
+	Dir   *File


There are 2 ways how to do this. Either we will include pointer on Parent here as you did, or we will use the State.ancestors to compute path to a marked file (something like State.Path() is doing) The benefit of your solution is that you can walk the folder tree in both directions.

Benefit of the alternative is that you still keep the File struct very simple and the tests including File are a bit simpler. Specially because if we wanted the test cases to reflect reality we couldn't put nil as a parent pointer we would have to put there the correct pattern.

What do you think about this @leonklingele?

Honestly I didn't even know that ancestors / state.Path() was a thing 😃
I'd go with my solution as it seems more "natural" and might simply things in the future.
Why do we need State.ancestors anyways? It's only used in a test.

That's a shame, this message had to get lost in the sheer amount of our conversation :) #18 (comment)

State.ancestors is used in GoBack command. It is different solution for the exact same problem (how to get from subdir to a parent dir)

My reasoning was that the file tree is going to have milions of files and every pointer/field I won't put into File struct is going to save a lot of space. When I compared memory footprint of godu with and without Dir in the File struct it increased (on my harddrive) from 180MB -> 203MB of memory.

I can't see an increase in RAM: For me it is 1.08 GB -> 1.05 GB Wow, that's pretty much! We definitely need to cut that down somehow.

The size of File struct without Dir is 56 bytes, after adding Dir the size increases to 64 bytes. I measured it using unsafe:

var file core.File unsafe.Sizeof(file)

I'm very surprised that You didn't notice any increase. There needs to be some factor missing.

If you insist that we are not going to use State.ancestors. And that the Dir variable is more clear and it's worth that 10%+ increase of memory footprint. I'm ok with having bi-directional file tree. I'd just ask you to rename Dir to something that will more represent the fact that it is a pointer to parent directory. Something like Parent, ParentDir, up to you.

This then bring work that the GoBack command needs to be changed and State.ancestors needs to be removed (because I don't want to have 2 different solutions for the same problem). I can do that or you can in another pull request, up to you. 😃

Renamed to Parent. I'll remove State.ancestors in a different MR.
Regarding space efficiency: We need to find other ways to reduce RAM usage.

leonklingele · 2017-01-10T20:13:58Z

Updated – once again. Please check aagain and let me know if it's ok for you.
If so, I will squash the commits in this MR.

viktomas

Please read through all the comments I left in my last review and let me know what you think.

Thanks so much for sticking with me for so long. I think that what code you did in the end is pretty awesome!

Please fix the test coverage, let me know what you think and I'm happy to merge this PR if you are OK with that.

viktomas · 2017-01-10T20:41:50Z

core/command.go

+	Folder      *File
+	Selected    int
+	history     map[*File]int // history of all selected postions
+	MarkedFiles map[*File]bool


This can be even more optimized (map[*File]struct{}) because boolean still takes up some space but empty struct does not (https://dave.cheney.net/2014/03/25/the-empty-struct). But this might be more readable :) I'm not too fussed, we don't need to change that.

Nice! struct{}{} looks ugly, but it's better than a bool, I agree. Updated.

viktomas · 2017-01-10T20:58:29Z

core/file_walker.go

 	"os"
 	"path/filepath"
 )

 type File struct {
 	Name  string
+	Dir   *File


The size of File struct without Dir is 56 bytes, after adding Dir the size increases to 64 bytes. I measured it using unsafe:

var file core.File unsafe.Sizeof(file)

I'm very surprised that You didn't notice any increase. There needs to be some factor missing.

If you insist that we are not going to use State.ancestors. And that the Dir variable is more clear and it's worth that 10%+ increase of memory footprint. I'm ok with having bi-directional file tree. I'd just ask you to rename Dir to something that will more represent the fact that it is a pointer to parent directory. Something like Parent, ParentDir, up to you.

This then bring work that the GoBack command needs to be changed and State.ancestors needs to be removed (because I don't want to have 2 different solutions for the same problem). I can do that or you can in another pull request, up to you. 😃

viktomas · 2017-01-10T21:01:59Z

core/file_walker_test.go

@@ -12,6 +12,7 @@ import (

 type fakeFile struct {
 	fileName  string
+	fileDir   *File


I don't think this is necessary, the fake file is representing "real" folder structure. It doesn't have to be consistent with core.File

Yup, removed.

viktomas · 2017-01-10T21:09:55Z

core/file_walker_test.go

-		t.Error("GetSubTree didn't return emtpy file on ReadDir failure")
+	name := "xyz"
+	result := GetSubTree(name, nil, failing, map[string]struct{}{})
+	want := File{Name: name}


You changed the logic here. This was testing that if the ReadDir function returns err (most likely because the file doesn't exist or we don't have access rights) the GetSubTree returns a File{} because that's the best it can do.

If ReadDir couldn't find xyz I don't think it's correct to return File{Name: "xyz"} because that file doesn't exist

I don't know why this was changed, but I can assure you that it had a good reason! Nvm, reverted :)

viktomas · 2017-01-10T21:17:45Z

core/processor.go

@@ -16,13 +16,15 @@ func PrepareTree(tree *File, limit int64) error {

 func StartProcessing(
 	folder *File,
-	commands chan Executer,
-	states chan State,
+	commands <-chan Executer,


marking channels as one-way is pretty neat, thanks!

Sorry, didn't mean to change it in this MR.

leonklingele · 2017-01-10T22:12:43Z

Updated, coverage is back to 100%. Squashed commits. Ready to be merged.

viktomas · 2017-01-10T22:32:24Z

Thanks @leonklingele! The README needs to be updated, let me know if you want to create another PR or I should do it.

viktomas requested changes Jan 8, 2017

View reviewed changes

leonklingele mentioned this pull request Jan 8, 2017

No updates until godu finishes full walk #14

Closed

leonklingele force-pushed the add-file-marking branch from d65415b to 618fd1a Compare January 9, 2017 22:55

leonklingele force-pushed the add-file-marking branch 5 times, most recently from 9d49392 to adc2d7b Compare January 10, 2017 02:02

viktomas requested changes Jan 10, 2017

View reviewed changes

viktomas approved these changes Jan 10, 2017

View reviewed changes

leonklingele added 2 commits January 10, 2017 23:12

Add support for file marking

6eb58a8

Fix existing & add new tests

00210c7

leonklingele force-pushed the add-file-marking branch from b69dbee to 00210c7 Compare January 10, 2017 22:12

viktomas merged commit 360842b into viktomas:master Jan 10, 2017

viktomas mentioned this pull request Jan 11, 2017

Writing tests is complicated #29

Closed

Add support for file marking #18

Add support for file marking #18

Conversation

leonklingele commented Jan 8, 2017

codecov-io commented Jan 8, 2017 • edited Loading

Current coverage is 100% (diff: 100%)

viktomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonklingele commented Jan 9, 2017

leonklingele commented Jan 10, 2017

viktomas commented Jan 10, 2017

viktomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonklingele commented Jan 10, 2017

viktomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonklingele commented Jan 10, 2017

viktomas commented Jan 10, 2017

codecov-io commented Jan 8, 2017 •

edited

Loading