Re: [PATCH 2/4] Library code for user-relative paths.
- From
Andreas Ericsson <ae@op5.se>
- Date
- Nov 2, 2005, 08:40 UTC
- Message-ID
- <43687B80.1020600@op5.se>
- In-Reply-To
- <7vk6fr6h3j.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano wrote:
Show 13 quoted lines
> 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.
Show 7 quoted lines
> >>+ 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?
Show 14 quoted lines
> >>+ 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