Re: [PATCH] Make git-clean a builtin

[Shawn Bohrer <shawn.bohrer@gmail.com>]

On Sat, Oct 06, 2007 at 11:52:53PM +0200, Frank Lichtenheld wrote:
> 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...

Agreed, I'm not sure what I was thinking there.

> > +	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?

If p == NULL then the path didn't have a EOL character.  This shouldn't
ever really happen since fgets() leaves the EOL character as part of the
string, and it is processing the output of git-ls-files which will
provide one path per line.  If it does happen for some reason then
either we will happily remove the file/directory, or if the path is
garbage then we will simply fail to remove anything.

> > +	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.

Yes, though unlikely I agree this wasn't very smart in the first place
so I fixed it and will send reworked patch soon.

--
Shawn