From: Andreas Ericsson <ae@op5.se>
Date: Wed, 02 Nov 2005 08:40:32 GMT
Subject: Re: [PATCH 2/4] Library code for user-relative paths.
Message-ID: <43687B80.1020600@op5.se>
In-Reply-To: <7vk6fr6h3j.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:
> Andreas Ericsson <ae@op5.se> writes:
> 
> 
>>+			if((slash = strchr(dir, '/'))) {
>>+				*slash = '\0';
>>+				pw = getpwnam(dir);
>>+				*slash = '/';
> 
> 
> Should you be writing into *slash when dir and path are const
> char *?  I know strchr returns "char *" and the compiler would
> not complain but this sounds somewhat yucky.
> 

True. Although path isn't, strictly speaking, const char *, so perhaps 
that's what needs fixing. It's only ever called with path coming from 
argv, which isn't const. I can't really imagine anywhere where the 
repo-path might be const char * now that I think of it.

> 
>>+		if(slash && *slash + 1)
> 
> 
> I think you mean "if (slash && slash[1])" here.  While we are at
> it, please have a SP betweeen if and open parenthesis.
> 

Actually *(slash + 1), but it amounts to the same thing I suppose. ;)

I looked around for indentation guide-lines but didn't found any, and 
the current code isn't exactly consistent about it. Perhaps it needs adding?

> 
>>+			dir = slash + 1;
>>+		else
>>+			dir = current_dir();
>>+	}
>>+
>>+	/* ~foo/path/to/repo is now path/to/repo and we're in foo's homedir */
>>+	if(chdir(dir) < 0)
>>+		return NULL;
> 
> 
> Hmm.  It's not wrong, but "dir = current_dir()" immediately
> followed by "chdir(dir)" does not feel right.
> 

It could be "return current_dir();" immediately, I suppose. Would that 
be satisfactory?

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231