From: Frank Lichtenheld <frank@lichtenheld.de>
Date: Sat, 06 Oct 2007 21:52:53 GMT
Subject: Re: [PATCH] Make git-clean a builtin
Message-ID: <20071006215253.GX31659@planck.djpig.de>
In-Reply-To: <11917040461528-git-send-email-shawn.bohrer@gmail.com>

On Sat, Oct 06, 2007 at 03:54:06PM -0500, Shawn Bohrer wrote:
> +static int remove_directory(const char *path)
> +{
> +	DIR *d;
> +	struct dirent *dir;
> +	d = opendir(path);
> +	if (d) {
> +		chdir(path);
> +		while ((dir = readdir(d)) != NULL) {
> +			if(strcmp( dir->d_name, ".") == 0 ||
> +			   strcmp( dir->d_name, ".." ) == 0 )
> +				continue;
> +			if (dir->d_type == DT_DIR)
> +				remove_directory(dir->d_name);
> +			else
> +				unlink(dir->d_name);
> +		}
> +	}
> +	closedir(d);
> +	chdir("..");
> +	return rmdir(path);
> +}

The unconditional chdir(..) after the conditional chdir(path) seems like
asking for trouble to me...

> +	while (fgets(path, sizeof(path), cmd_fout) != NULL) {
> +		struct stat st;
> +		char *p;
> +		p = strrchr(path, '\n');
> +		if ( p != NULL )
> +			*p = '\0';

What happens in case p == NULL? It simply tries to remove the partial
path?

> +	fclose(cmd_fout);
> +	finish_command(&cmd);
> +	if (!ignored && !access(git_path("info/exclude"), F_OK))
> +		free(buf);

There is a race condition here of the value of access() changes between
the two calls. Not one likely to trigger but it should be easy to avoid
alltogether.

> +	free(argv_ls_files);
> +	return 0;
> +}

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/