Jari Aalto <jari.aalto@xxxxxxxxx> writes:
> * str_replace(): New function. Generic replace command.
> * str_replace_home(): New function. Substitute $HOME and tilde(~) in string.
> * git_default_config(): Pass core.excludesfile to str_replace_home().
>
> Signed-off-by: Jari Aalto <jari.aalto AT cante.net>
First, git project does NOT use GNU ChangeLog convention for it's
commit messages. We rather use descriptive commit messages.
Second, I'm not sure about str_replace... how it fits with strbufs?
AFAIK we try to use strbufs whenever possible and feasible, to avoid
errors in git.
Third, I agree that it is a good idea, but I'd rather have *full*
solution, i.e. for git to expand $HOME (or better yet any
environmental variable) and '~' everywhere, not only for
core.excludesfile, but also for --git-dir and GIT_DIR, for
core.worktree and --work-tree and GIT_WORK_TREE, and for all other
config variables and enviroment variables.
> ---
>
> From ac6941f5055b2acdded59627d228bbf03ba0d9fc
What does it mean? A bit cryptic, don't you think?
Comments on code below. One thing: we use tabs for indent, not
spaces. You use spaces in your code, while context uses tabs.
> +char *str_replace(const char *str, const char *find, const char *replace)
> +{
> + int maxlen = strlen(str) + strlen(replace) + 1;
> + char *start = strstr(str, find);
> + char *ptr = (char *)malloc(maxlen);
> + int len = strlen(find);
> + int llen, rlen; /* left, right portion length */
> +
> + if (start == (char *)NULL) {
There is no need to cast NULL. Besides, we write IIRC "if (!start)",
it is common enough idiom.
> + strcpy( ptr, str);
Style: no space after opening parenthesis: "strcpy(ptr, str)".
Performance: I think it would be better to use stpcpy, although I'm
not quite sure if here too.
> + }
> + else{
> + rlen = strlen(start) - strlen(find);
> + llen = strlen(str) - strlen(start);
> + strncpy( ptr, str, llen);
> + strcat( ptr, replace);
> + strncat( ptr, start + len, rlen); /* Does not add '\0' */
> + strcat( ptr, ""); /* Terminate with null string */
Performance suck here; although this is not time-critical path using
sequence of strcat is just bad style. And 'strcat( ptr, "");' just
takes the cake: use "ptr[len] = '\0'", ehere 'len' is calculated
length of string.
P.S. Junio, when do you think 1.5.4 would be finally released? We have
feature freeze still, isn't it?
--
Jakub Narebski
Poland
ShadeHawk on #git
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
