Matthieu Moy <Matthieu.Moy@xxxxxxx> writes:
> My version is made a bit simpler by using strbuf for string
> manipulation in expand_user_path.
Nice to see people keeping track of issues that we tried to address but
didn't complete.
> I'm not sure I fully adressed Junio's point here:
We'll see ;-)
> I'm just copying back into the static buffer to let enter_repo() do
> the same string manipulation as it used to do (concatenate with .git
> suffixes). I think the whole enter_repo could use strbuf instead of
> static buffers, but that's a different point (probably made easier by
> my patch).
> diff --git a/config.c b/config.c
> index c644061..0fcc4ce 100644
> --- a/config.c
> +++ b/config.c
> @@ -351,6 +351,15 @@ int git_config_string(const char **dest, const char *var, const char *value)
> return 0;
> }
>
> +int git_config_pathname(const char **dest, const char *var, const char *value) {
> + if (!value)
> + return config_error_nonbool(var);
> + *dest = expand_user_path(value);
> + if (!*dest)
> + die("Failed to expand user dir in: '%s'", value);
> + return 0;
> +}
> +
> @@ -207,43 +208,49 @@ int validate_headref(const char *path)
> return -1;
> }
>
> -static char *user_path(char *buf, char *path, int sz)
> +static inline struct passwd *getpw_str(const char *username, size_t len)
> {
> + if (len == 0)
> + return getpwuid(getuid());
> +
> struct passwd *pw;
Decl-after-statement.
Do you need this special case of passing a zero-length string (not NULL
pointer as a string) to getpw_str() to grab the current user? Which
codepath is this needed?
> + char *username_z = xmalloc(len + 1);
> + memcpy(username_z, username, len);
> + username_z[len] = '\0';
> + pw = getpwnam(username_z);
> + free(username_z);
> + return pw;
> +}
> +/*
> + * Return a string with ~ and ~user expanded via getpw*. If buf != NULL, then
> + * it is a newly allocated string. Returns NULL on getpw failure or if
> + * path is NULL.
> + */
> +char *expand_user_path(const char *path)
> +{
> + struct strbuf user_path = STRBUF_INIT;
> + char * first_slash = strchrnul(path, '/');
> + char * to_copy;
Style: "char *first_slash"
Should't "to_copy" be initialized to "path" here? What do you copy when
path[0] is '/'?
> + if (path == NULL)
> + goto return_null;
> +
> + if (path[0] == '~') {
> + const char *username = path + 1;
> + size_t username_len = first_slash - username;
> + struct passwd *pw = getpw_str(username, username_len);
> + if (!pw)
> + goto return_null;
> + strbuf_add(&user_path, pw->pw_dir, strlen(pw->pw_dir));
> + to_copy = first_slash;
> + } else if (path[0] != '/') {
> + to_copy = path;
> }
> - return buf;
> + strbuf_add(&user_path, to_copy, strlen(to_copy));
> + return strbuf_detach(&user_path, NULL);
> +return_null:
> + strbuf_release(&user_path);
> + return NULL;
> }
> /*
> @@ -291,8 +298,17 @@ char *enter_repo(char *path, int strict)
> if (PATH_MAX <= len)
> return NULL;
> if (path[0] == '~') {
> - if (!user_path(used_path, path, PATH_MAX))
> + char *newpath = expand_user_path(path);
> + if (!newpath || (PATH_MAX - 10 < strlen(newpath))) {
> + if (path != newpath)
> + free(newpath);
Your expand_user_path() never returns the original, no?
> return NULL;
> + }
> + /* Copy back into the static buffer. A pity
> + since newpath was not bounded, but other
> + branches of the if are limited by PATH_MAX
> + anyway. */
> + strcpy(used_path, newpath); free(newpath);
> strcpy(validated_path, path);
> path = used_path;
> }
Heh, in a sense you _did_ address my point by punting, but that is Ok. As
you said earlier that would be a good topic of a separate patch.
/*
* By the way, we write our
* multi-line comments like
* this.
*/
--
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