Cleanup files.PublishedStorage after dropping a publish

[aol-nnov]

Fixes: #198

After dropping files.PublishedStorage there were some leftovers - empty directory tree. If that directory tree is empty, it is now removed.

Checklist

• unit-test added (if change is algorithm)
• functional test added/updated (if change is functional)

[aol-nnov]

@neolynx here is another annoying thing I'd like to fix in aptly. My publish prefix is bloated with empty directories as I have to create and remove publishes intensively.

[neolynx]

oh, yes please ! I found this quite annoying as well...

[aol-nnov]

yay, there is an issue for it! #198

[neolynx]

could we rather check if it is empty before removing ?

[aol-nnov]

We could, in theory, but it is a lot more code, than checking an error...

[neolynx]

i know, but it is also more proper, to not accidentally delete stuff :)

[aol-nnov]

I do not agree with you. Also, it is algorithmically more complex to perform the check you propose.

In general, the question is: why do you trust one syscall and do not trust another?

[neolynx]

I think it is a question of taste... ;-) in some industries it is even not allowed to rely on error text, ... not a question of trusting sys calls..

in aptly I would prefer the check before action, we could add a function to utils/utils.go:

func isDirEmpty(path string) (bool, error) {
	f, err := os.Open(path)
	if err != nil {
		return false, err
	}
	defer f.Close()

	contents, err := f.Readdirnames(1) // Read only 1 file to check if it's empty
	if err != nil {
		return false, err
	}

	return len(contents) == 0, nil
}

and just call this before the os.Remove call. I would not worry about algorithic complexity with a low number of nested directories..

what do you think ?

[aol-nnov]

Look, even if os.Remove broke and start removing non-empty directories, it won't go beyond publish root. Also, if any of publishes are accidentally lost (which I highly doubt), they could be recreated from corresponding repos.

Anyway, I'm out of office for today and do not have a laptop at hand. Let's get back to it tomorrow.

[aol-nnov]

Also, if the root of your comment is that it is no good checking error text, we could simply bail out on any error. It is perfectly fine also.

[neolynx]

Look, the file system might also be on a NFS server, and I would like aptly to check before a remove action and have syscalls return errors on real errors. Yes it works both ways, but here in aptly i feel we should do it this way ;-)

Aside from this, there are some tests failing, maybe the tests were wrong, relying on the empty directory ?