Re: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 16, 2009, 22:49 UTC
- Message-ID
- <7v3a4enkzt.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1258366060-27966-1-git-send-email-Matthieu.Moy@imag.fr>
Matthieu Moy <Matthieu.Moy@imag.fr> 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 ;-)
Show 5 quoted lines
> 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).
Show 17 quoted lines
> 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;
> +}
> +Show 11 quoted lines
> @@ -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?
Show 7 quoted lines
> + char *username_z = xmalloc(len + 1); > + memcpy(username_z, username, len); > + username_z[len] = '\0'; > + pw = getpwnam(username_z); > + free(username_z); > + return pw; > +}
Show 10 quoted lines
> +/*
> + * 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 '/'?
Show 21 quoted lines
> + 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;
> }Show 10 quoted lines
> /*
> @@ -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?
Show 10 quoted lines
> 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.
*/