[PATCH] osutil: implementation for Win32

[Matt Mackall]

On Tue, 2008-09-09 at 20:51 +0200, Adrian Buehlmann wrote:
> # HG changeset patch
> # User Petr Kodl <petrkodl at gmail.com>
> # Date 1220959680 14400
> # Node ID 020fe75b47f569a29c25f8a5fef7451100d72772
> # Parent  9141bebefe3ed0498ff07271214bfca8b4eb0d27
> osutil: implementation for Win32

Thanks, guys. This is looking much better. A few more comments, I'm
afraid.

> +static PyObject *listdir(PyObject *self, PyObject *args, PyObject *kwargs)
> +{
> +	PyObject *statobj = NULL;
> +	PyObject *list = NULL;
> +	PyObject *items = NULL;
> +	PyObject *ctor_args = NULL;
> +	PyObject *item0 = NULL;
> +	PyObject *item1 = NULL;
> +	PyObject *py_st = NULL;

There's a slightly weird thing going on here with initializing
variables. I eventually figured out it's because of a misunderstanding
of how the 'goto' error-handling style works. It's supposed to work
roughly like this:

	a = malloc(x);
	ret = NO_MEMORY;
	if (!a)
		goto error_a;

	b = open(y);
	ret = OPEN_FAILED;
	if (!b)
		goto error_b;

	 ret = OOPS;
	do { 
		if (!trysomething(a,b))
			goto error;
		...
	}
	ret = SUCCESS;

error: /* need to free all resources */
	close(b);
error_b: /* don't need to close b, because we didn't open it */
	free(a);
error_a: /* don't need to free a */
	return ret;

The idea is that you can bail out easily anywhere in your function even
if there are already some resources allocated that need cleaning up and
not have the error handling paths dominate your code or cause massive
indenting. 

To make this work, you don't have to do anything special with variable
initialization (and you don't have to unset things at the end, either!),
you just have to a entry point in the 'unrolling' for every 'block' of
resources you allocate.

> +	if (INVALID_HANDLE_VALUE == fh) {

I'm not a fan of this backwards comparison style, btw.

> +		int isro  = fd.dwFileAttributes & FILE_ATTRIBUTE_READONLY;

Read-only isn't meaningful to Mercurial.

> +		if (isdir && (!strcmp(fd.cFileName, ".")
> +						|| !strcmp(fd.cFileName, "..")))
> +			continue;
> +
> +		items = PyTuple_New(keepstat ? 3 : 2);
> +
> +		item0 = PyString_FromString(fd.cFileName);
> +		item1 = PyInt_FromLong(isdir ? _S_IFDIR : _S_IFREG);
> +
> +		if (!items || !item0 || !item1) {
> +			PyErr_NoMemory();
> +			goto error;
> +		}
> +
> +		PyTuple_SetItem(items, 0, item0);
> +		PyTuple_SetItem(items, 1, item1);
> +		item0 = item1 = NULL;
> +

I know this was probably cut-and-pasted from the original, but this
would probably be better with something like:

if (keepstat) {
	...
	items = Py_BuildValue("siO", fd.cFileName, mode, py_st);
} else 
	items = Py_BuildValue("si", fd.cFileName, mode);

if (!items)
	...

That is, we can get rid of a bunch of single object creates, item sets, 
and error checking.

-- 
Mathematics is the supreme nostalgia of our time.