threads / patch / 21635

patchExpand ~ and ~user in core.excludesfile, commit.template

Subject: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template

## tl;dr

24 messages between Nov 16, 2009 and Nov 19, 2009. Diffs are folded; open one to read it.

replies: 23people: 7as markdown or json

Matthieu Moy· Nov 16, 2009, 10:07 UTC · lore

These config variables are parsed to substitute ~ and ~user with getpw entries.

user_path() refactored into new function expand_user_path(), to allow dynamically allocating the return buffer.

Original patch by Karl Chen, modified by Matthieu Moy.
Signed-off-by: Karl Chen <quarl@quarl.org>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
The original thread was here:
http://thread.gmane.org/gmane.comp.version-control.git/93250/focus=94276

My version is made a bit simpler by using strbuf for string manipulation in expand_user_path.

I'm not sure I fully adressed Junio's point here: http://thread.gmane.org/gmane.comp.version-control.git/93250/focus=94276

Show 15 quoted lines
> I do not see any strong reason why the single caller of user_path() has to
> keep using the static allocation.  Would it help to reduce the complexity
> of your expand_user_path() implementation, if we fixed the caller along
> the lines of this patch (untested, but just to illustrate the point)?
> 
> [...]
> --- i/path.c
> +++ w/path.c
> @@ -221,19 +221,22 @@ 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);
> [...]

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).

 Documentation/config.txt |    4 ++-
 builtin-commit.c         |    2 +-
 cache.h                  |    2 +
 config.c                 |   11 +++++-
 path.c                   |   84 +++++++++++++++++++++++++++------------------
 5 files changed, 66 insertions(+), 37 deletions(-)
Show changes to 5 files +66 −37

Documentation/config.txt, builtin-commit.c, cache.h, config.c, path.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d1e2120..c37b51d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -380,7 +380,8 @@ Common unit suffixes of 'k', 'm', or 'g' are supported.
 core.excludesfile::
 	In addition to '.gitignore' (per-directory) and
 	'.git/info/exclude', git looks into this file for patterns
-	of files which are not meant to be tracked.  See
+	of files which are not meant to be tracked.  "~" and "~user"
+	are expanded to the user's home directory.  See
 	linkgit:gitignore[5].
 
 core.editor::
@@ -670,6 +671,7 @@ color.ui::
 
 commit.template::
 	Specify a file to use as the template for new commit messages.
+	"~" and "~user" are expanded to the user's home directory.
 
 diff.autorefreshindex::
 	When using 'git-diff' to compare with work tree
diff --git a/builtin-commit.c b/builtin-commit.c
index d525b89..09d2840 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -999,7 +999,7 @@ static int git_commit_config(const char *k, const char *v, void *cb)
 	struct wt_status *s = cb;
 
 	if (!strcmp(k, "commit.template"))
-		return git_config_string(&template_file, k, v);
+		return git_config_pathname(&template_file, k, v);
 
 	return git_status_config(k, v, s);
 }
diff --git a/cache.h b/cache.h
index 71a731d..42f7cd8 100644
--- a/cache.h
+++ b/cache.h
@@ -645,6 +645,7 @@ int set_shared_perm(const char *path, int mode);
 #define adjust_shared_perm(path) set_shared_perm((path), 0)
 int safe_create_leading_directories(char *path);
 int safe_create_leading_directories_const(const char *path);
+extern char *expand_user_path(const char *path);
 char *enter_repo(char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {
@@ -903,6 +904,7 @@ extern unsigned long git_config_ulong(const char *, const char *);
 extern int git_config_bool_or_int(const char *, const char *, int *);
 extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
+extern int git_config_pathname(const char **, const char *, const char *);
 extern int git_config_set(const char *, const char *);
 extern int git_config_set_multivar(const char *, const char *, const char *, int);
 extern int git_config_rename_section(const char *, const char *);
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;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */
@@ -474,7 +483,7 @@ static int git_default_core_config(const char *var, const char *value)
 		return git_config_string(&editor_program, var, value);
 
 	if (!strcmp(var, "core.excludesfile"))
-		return git_config_string(&excludes_file, var, value);
+		return git_config_pathname(&excludes_file, var, value);
 
 	if (!strcmp(var, "core.whitespace")) {
 		if (!value)
diff --git a/path.c b/path.c
index 047fdb0..009c8e0 100644
--- a/path.c
+++ b/path.c
@@ -11,6 +11,7 @@
  * which is what it's designed for.
  */
 #include "cache.h"
+#include "strbuf.h"
 
 static char bad_path[] = "/bad-path/";
 
@@ -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;
-	char *slash;
-	int len, baselen;
+	char *username_z = xmalloc(len + 1);
+	memcpy(username_z, username, len);
+	username_z[len] = '\0';
+	pw = getpwnam(username_z);
+	free(username_z);
+	return pw;
+}
 
-	if (!path || path[0] != '~')
-		return NULL;
-	path++;
-	slash = strchr(path, '/');
-	if (path[0] == '/' || !path[0]) {
-		pw = getpwuid(getuid());
-	}
-	else {
-		if (slash) {
-			*slash = 0;
-			pw = getpwnam(path);
-			*slash = '/';
-		}
-		else
-			pw = getpwnam(path);
-	}
-	if (!pw || !pw->pw_dir || sz <= strlen(pw->pw_dir))
-		return NULL;
-	baselen = strlen(pw->pw_dir);
-	memcpy(buf, pw->pw_dir, baselen);
-	while ((1 < baselen) && (buf[baselen-1] == '/')) {
-		buf[baselen-1] = 0;
-		baselen--;
-	}
-	if (slash && slash[1]) {
-		len = strlen(slash);
-		if (sz <= baselen + len)
-			return NULL;
-		memcpy(buf + baselen, slash, len + 1);
+/*
+ * 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;
+	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);
 				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;
 		}
-- 
1.6.5.2.152.gbbe9e
Junio C Hamano· Nov 16, 2009, 22:49 UTC · re: Matthieu Moy · lore

Re: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template

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.
         */
Junio C Hamano· Nov 17, 2009, 06:49 UTC · re: Junio C Hamano · lore

Re: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template

I'd like to squash this to your patch, based on the earlier review comments.

I didn't mention about the first hunk; it is just a style. An opening brace that begins a function body comes at column 1.

Also first_slash and to_copy are made const pointers, as they do not have to touch the region of memory they point to (otherwise you cannot assign path to to_copy without getting warned).

 config.c |    3 ++-
 path.c   |   32 ++++++++++++++------------------
 2 files changed, 16 insertions(+), 19 deletions(-)
Show changes to 2 files +16 −19

config.c, path.c

diff --git a/config.c b/config.c
index 0fcc4ce..b3d1ff4 100644
--- a/config.c
+++ b/config.c
@@ -351,7 +351,8 @@ 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) {
+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);
diff --git a/path.c b/path.c
index 009c8e0..2470f78 100644
--- a/path.c
+++ b/path.c
@@ -208,11 +208,8 @@ int validate_headref(const char *path)
 	return -1;
 }
 
-static inline struct passwd *getpw_str(const char *username, size_t len)
+static struct passwd *getpw_str(const char *username, size_t len)
 {
-	if (len == 0)
-		return getpwuid(getuid());
-
 	struct passwd *pw;
 	char *username_z = xmalloc(len + 1);
 	memcpy(username_z, username, len);
@@ -223,18 +220,18 @@ static inline struct passwd *getpw_str(const char *username, size_t len)
 }
 
 /*
- * 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.
+ * 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;
+	const char *first_slash = strchrnul(path, '/');
+	const char *to_copy = path;
+
 	if (path == NULL)
 		goto return_null;
-
 	if (path[0] == '~') {
 		const char *username = path + 1;
 		size_t username_len = first_slash - username;
@@ -243,8 +240,6 @@ char *expand_user_path(const char *path)
 			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;
 	}
 	strbuf_add(&user_path, to_copy, strlen(to_copy));
 	return strbuf_detach(&user_path, NULL);
@@ -300,14 +295,15 @@ char *enter_repo(char *path, int strict)
 		if (path[0] == '~') {
 			char *newpath = expand_user_path(path);
 			if (!newpath || (PATH_MAX - 10 < strlen(newpath))) {
-				if (path != newpath)
-					free(newpath);
+				free(newpath);
 				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. */
+			/*
+			 * 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;
-- 
1.6.5.3.283.g4b054
Matthieu Moy· Nov 17, 2009, 08:59 UTC · re: Junio C Hamano · lore

Re: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template

Junio C Hamano <gitster@pobox.com> writes:
> I'd like to squash this to your patch, based on the earlier review
> comments.

Sure. And apologize for the style issues, I knew it, but one hardly changes his (bad) habits ;-).

I'll resend soon.
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Jakub Narebski· Nov 16, 2009, 23:47 UTC · re: Matthieu Moy · lore

Re: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template

Matthieu Moy <Matthieu.Moy@imag.fr> writes:
Show 11 quoted lines
> These config variables are parsed to substitute ~ and ~user with getpw
> entries.
> 
> user_path() refactored into new function expand_user_path(), to allow
> dynamically allocating the return buffer.
> 
> Original patch by Karl Chen, modified by Matthieu Moy.
> 
> Signed-off-by: Karl Chen <quarl@quarl.org>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
Show 14 quoted lines
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index d1e2120..c37b51d 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -380,7 +380,8 @@ Common unit suffixes of 'k', 'm', or 'g' are supported.
>  core.excludesfile::
>  	In addition to '.gitignore' (per-directory) and
>  	'.git/info/exclude', git looks into this file for patterns
> -	of files which are not meant to be tracked.  See
> +	of files which are not meant to be tracked.  "~" and "~user"
> +	are expanded to the user's home directory.  See
>  	linkgit:gitignore[5].
>  
>  core.editor::

It would be nice to have an option to git-config which would do such expansion, as a separate type similar to --int and --bool, e.g.:

  git config --path section.key

so that not only core.excludesfile could use this new feature, but for example also core.worktree, commit.template, gitcvs.logfile, mailmap.file, and perhaps also *.receivepack and *.uploadpack

-- 
Jakub Narebski
Poland
ShadeHawk on #git
Junio C Hamano· Nov 17, 2009, 06:22 UTC · re: Jakub Narebski · lore

Re: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template

Jakub Narebski <jnareb@gmail.com> writes:
Show 23 quoted lines
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index d1e2120..c37b51d 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -380,7 +380,8 @@ Common unit suffixes of 'k', 'm', or 'g' are supported.
>>  core.excludesfile::
>>  	In addition to '.gitignore' (per-directory) and
>>  	'.git/info/exclude', git looks into this file for patterns
>> -	of files which are not meant to be tracked.  See
>> +	of files which are not meant to be tracked.  "~" and "~user"
>> +	are expanded to the user's home directory.  See
>>  	linkgit:gitignore[5].
>>  
>>  core.editor::
>
> It would be nice to have an option to git-config which would do such
> expansion, as a separate type similar to --int and --bool, e.g.:
>
>   git config --path section.key
>
> so that not only core.excludesfile could use this new feature, but for
> example also core.worktree, commit.template, gitcvs.logfile,
> mailmap.file, and perhaps also *.receivepack and *.uploadpack
What should "git config -l" do for these (and core.excludesfile)?
Matthieu Moy· Nov 17, 2009, 08:57 UTC · re: Junio C Hamano · lore

Re: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> Jakub Narebski <jnareb@gmail.com> writes:
>
>> It would be nice to have an option to git-config which would do such
>> expansion, as a separate type similar to --int and --bool, e.g.:
>>
>>   git config --path section.key
>>
>> so that not only core.excludesfile could use this new feature, but for
>> example also core.worktree, commit.template, gitcvs.logfile,
>> mailmap.file, and perhaps also *.receivepack and *.uploadpack
>
> What should "git config -l" do for these (and core.excludesfile)?

I don't know what it "should", but it "does" not do the expansion. I had the same questionning when testing the patch, I'd have liked to be able to write a simple test-case like

$ git config core.excludesfile '~/foo' $ git config --i-dont-know-what core.excludesfile

to go through this codepath. Maybe we can just say
$ git config --default core.excludesfile

to say "call git_default_config(...) on this before printing it". My understanding is that this is what the C code is doing, we should allow the shell scripts to do the same.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Jakub Narebski· Nov 17, 2009, 13:30 UTC · re: Matthieu Moy · lore

Re: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template

Dnia wtorek 17. listopada 2009 09:57, Matthieu Moy napisał:
Show 28 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>> Jakub Narebski <jnareb@gmail.com> writes:
>>
>>> It would be nice to have an option to git-config which would do such
>>> expansion, as a separate type similar to --int and --bool, e.g.:
>>>
>>>   git config --path section.key
>>>
>>> so that not only core.excludesfile could use this new feature, but for
>>> example also core.worktree, commit.template, gitcvs.logfile,
>>> mailmap.file, and perhaps also *.receivepack and *.uploadpack
>>
>> What should "git config -l" do for these (and core.excludesfile)?
> 
> I don't know what it "should", but it "does" not do the expansion. I
> had the same questionning when testing the patch, I'd have liked to be
> able to write a simple test-case like
> 
> $ git config core.excludesfile '~/foo'
> $ git config --i-dont-know-what core.excludesfile
> 
> to go through this codepath. Maybe we can just say
> 
> $ git config --default core.excludesfile
> 
> to say "call git_default_config(...) on this before printing it". My
> understanding is that this is what the C code is doing, we should
> allow the shell scripts to do the same.

I think it is a very good idea. Nevertheless it can apply only to config variables git core knows about, and not for example for git-gui, or gitk, or qgit, or tig, or StGIT, etc. configuration. Therefore "git config --path" would be still needed.

-- 
Jakub Narębski
Poland
Jakub Narebski· Nov 17, 2009, 09:30 UTC · re: Junio C Hamano · lore

Re: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template

Dnia wtorek 17. listopada 2009 07:22, Junio C Hamano napisał:
> Jakub Narebski <jnareb@gmail.com> writes:
Show 11 quoted lines
> >
> > It would be nice to have an option to git-config which would do such
> > expansion, as a separate type similar to --int and --bool, e.g.:
> >
> >   git config --path section.key
> >
> > so that not only core.excludesfile could use this new feature, but for
> > example also core.worktree, commit.template, gitcvs.logfile,
> > mailmap.file, and perhaps also *.receivepack and *.uploadpack
> 
> What should "git config -l" do for these (and core.excludesfile)?

Nothing (i.e. show unexpanded / not converted), just like for boolean variables "git config -l" doesn't convert 1/on/yes to true, and 0/off/no to false, just like for integer variables "git config -l" doesn't convert to simple decimal number taking into account optional 'k', 'm' or 'g' suffix.

BTW. the suffix part of integer conversion should really be described in the paragraph about --int and --bool options (which should be made into proper description list, and not a prose paragraph).

P.S. I am a bit missing --local / --repository option to git-config to complement --global (which should probably be named --user) and --system options.

-- 
Jakub Narebski
Poland
Jeff King· Nov 17, 2009, 07:34 UTC · re: Matthieu Moy · lore

Re: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template

On Mon, Nov 16, 2009 at 11:07:40AM +0100, Matthieu Moy wrote:
Show 12 quoted lines
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index d1e2120..c37b51d 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -380,7 +380,8 @@ Common unit suffixes of 'k', 'm', or 'g' are supported.
>  core.excludesfile::
>  	In addition to '.gitignore' (per-directory) and
>  	'.git/info/exclude', git looks into this file for patterns
> -	of files which are not meant to be tracked.  See
> +	of files which are not meant to be tracked.  "~" and "~user"
> +	are expanded to the user's home directory.  See
>  	linkgit:gitignore[5].
Reading this, it is not clear to me if:
  1. "~" and "~user" are expanded to the home directory of "user", where
     "user" is the user running git
or
  2. "~" is expanded to the home directory of the user running git, and
     an arbitrary "~user" is expanded to that user's home directory.

I would expect (2), since that is how everything else works. And it seems from the code that is what you do. But something about the way it is written makes me think of (1). I also recall having this same question during the last round, so at the very least it is not me just mis-reading it once. ;)

Maybe:
  A leading path component of "~user" is expanded to the home directory
  of "user"; "~" is expanded to the home directory of the user running
  git.
would be more clear?
-Peff
Mike Hommey· Nov 17, 2009, 07:49 UTC · re: Jeff King · lore

Re: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template

On Tue, Nov 17, 2009 at 02:34:26AM -0500, Jeff King wrote:
Show 7 quoted lines
> Maybe:
> 
>   A leading path component of "~user" is expanded to the home directory
>   of "user"; "~" is expanded to the home directory of the user running
>   git.
> 
> would be more clear?

Add "real" before "user running git" and you have my vote. Or maybe using the effective user would be better, and the patch should use geteuid() instead of getuid(), I don't know. ident.c uses getuid(), but I'm wondering if that's what it should use (although it doesn't seem to have been a problem to anyone)

Mike
Andreas Schwab· Nov 17, 2009, 21:20 UTC · re: Mike Hommey · lore

Re: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template

Mike Hommey <mh@glandium.org> writes:
Show 14 quoted lines
> On Tue, Nov 17, 2009 at 02:34:26AM -0500, Jeff King wrote:
>> Maybe:
>> 
>>   A leading path component of "~user" is expanded to the home directory
>>   of "user"; "~" is expanded to the home directory of the user running
>>   git.
>> 
>> would be more clear?
>
> Add "real" before "user running git" and you have my vote. Or maybe
> using the effective user would be better, and the patch should use
> geteuid() instead of getuid(), I don't know. ident.c uses getuid(), but
> I'm wondering if that's what it should use (although it doesn't seem to
> have been a problem to anyone)

"~" should just expand to the value of $HOME, like in the shell, independent of the real home directory of the real or effective user.

Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Junio C Hamano· Nov 17, 2009, 22:16 UTC · re: Andreas Schwab · lore

Re: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template

Andreas Schwab <schwab@linux-m68k.org> writes:
Show 19 quoted lines
> Mike Hommey <mh@glandium.org> writes:
>
>> On Tue, Nov 17, 2009 at 02:34:26AM -0500, Jeff King wrote:
>>> Maybe:
>>> 
>>>   A leading path component of "~user" is expanded to the home directory
>>>   of "user"; "~" is expanded to the home directory of the user running
>>>   git.
>>> 
>>> would be more clear?
>>
>> Add "real" before "user running git" and you have my vote. Or maybe
>> using the effective user would be better, and the patch should use
>> geteuid() instead of getuid(), I don't know. ident.c uses getuid(), but
>> I'm wondering if that's what it should use (although it doesn't seem to
>> have been a problem to anyone)
>
> "~" should just expand to the value of $HOME, like in the shell,
> independent of the real home directory of the real or effective user.

How should this interact with installations that run gitosis/gitlite that use shared account, giving user identity via the ssh key?

Note that the question is not "how would this...", but "how _should_ this...".

Andreas Schwab· Nov 18, 2009, 00:42 UTC · re: Junio C Hamano · lore

Re: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template

Junio C Hamano <gitster@pobox.com> writes:
Show 24 quoted lines
> Andreas Schwab <schwab@linux-m68k.org> writes:
>
>> Mike Hommey <mh@glandium.org> writes:
>>
>>> On Tue, Nov 17, 2009 at 02:34:26AM -0500, Jeff King wrote:
>>>> Maybe:
>>>> 
>>>>   A leading path component of "~user" is expanded to the home directory
>>>>   of "user"; "~" is expanded to the home directory of the user running
>>>>   git.
>>>> 
>>>> would be more clear?
>>>
>>> Add "real" before "user running git" and you have my vote. Or maybe
>>> using the effective user would be better, and the patch should use
>>> geteuid() instead of getuid(), I don't know. ident.c uses getuid(), but
>>> I'm wondering if that's what it should use (although it doesn't seem to
>>> have been a problem to anyone)
>>
>> "~" should just expand to the value of $HOME, like in the shell,
>> independent of the real home directory of the real or effective user.
>
> How should this interact with installations that run gitosis/gitlite that
> use shared account, giving user identity via the ssh key?

Sorry, I don't know enough about such an installation to be able to answer that.

Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Matthieu Moy· Nov 18, 2009, 07:24 UTC · re: Junio C Hamano · lore

Re: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template

Show 6 quoted lines
> Andreas Schwab <schwab@linux-m68k.org> writes:
>
>> Mike Hommey <mh@glandium.org> writes:
>>
>> "~" should just expand to the value of $HOME, like in the shell,
>> independent of the real home directory of the real or effective user.
Right, I'll resend in a minute.
Junio C Hamano <gitster@pobox.com> writes:
> How should this interact with installations that run gitosis/gitlite that
> use shared account, giving user identity via the ssh key?

I guess there's some kind of rhetorical question behind that I missed, but if a gitosis-like installation uses the user git, and the user foo connects to it identifier by its SSH key, I don't see what you could do other than expanding ~ to the homedir of git, which is $HOME at the time your run git on the server.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Matthieu Moy· Nov 17, 2009, 08:53 UTC · re: Jeff King · lore

Re: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template

Jeff King <peff@peff.net> writes:
Show 17 quoted lines
> On Mon, Nov 16, 2009 at 11:07:40AM +0100, Matthieu Moy wrote:
>
>> +	of files which are not meant to be tracked.  "~" and "~user"
>> +	are expanded to the user's home directory.  See
>>  	linkgit:gitignore[5].
>
> Reading this, it is not clear to me if:
>
>   1. "~" and "~user" are expanded to the home directory of "user", where
>      "user" is the user running git
>
> or
>
>   2. "~" is expanded to the home directory of the user running git, and
>      an arbitrary "~user" is expanded to that user's home directory.
>
> I would expect (2), since that is how everything else works.

Yes. "user" in the sentence is either the user running Git or the same string as "user" in "~user". I'm not against your proposal, but I'm afraid we're making the sentence uselessly heavy, since, as you say, this ~ and ~user convention is widely spread, and I hardly imagine someone interpreting the sentence as "if you say ~foo, it will expand to the home directory of bar".

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Jeff King· Nov 17, 2009, 08:56 UTC · re: Matthieu Moy · lore

Re: [PATCH] Expand ~ and ~user in core.excludesfile, commit.template

On Tue, Nov 17, 2009 at 09:53:52AM +0100, Matthieu Moy wrote:
Show 6 quoted lines
> Yes. "user" in the sentence is either the user running Git or the same
> string as "user" in "~user". I'm not against your proposal, but I'm
> afraid we're making the sentence uselessly heavy, since, as you say,
> this ~ and ~user convention is widely spread, and I hardly imagine
> someone interpreting the sentence as "if you say ~foo, it will expand
> to the home directory of bar".

I didn't think it would expand ~foo to the home directory of bar. I thought that it might _only_ accept ~bar, and not ~foo.

If I'm the only one confused, then we can drop it. But if not, I don't think it is much more work to be precise.

-Peff
Matthieu Moy· Nov 17, 2009, 17:24 UTC · re: Matthieu Moy · lore

[PATCH v2] Expand ~ and ~user in core.excludesfile, commit.template

These config variables are parsed to substitute ~ and ~user with getpw entries.

user_path() refactored into new function expand_user_path(), to allow dynamically allocating the return buffer.

Original patch by Karl Chen, modified by Matthieu Moy, and further amended by Junio C Hamano.

Signed-off-by: Karl Chen <quarl@quarl.org>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Here's the new version, with Junio's modifications squashed in.

I just replaced "user" with "specified user" to make it clear one can specify a user with ~foo/, but since the documentation appears in two places (possibly more in the future), I prefered to Keep It Simple.

 Documentation/config.txt |    4 ++-
 builtin-commit.c         |    2 +-
 cache.h                  |    2 +
 config.c                 |   12 ++++++-
 path.c                   |   80 ++++++++++++++++++++++++++-------------------
 5 files changed, 63 insertions(+), 37 deletions(-)
Show changes to 5 files +63 −37

Documentation/config.txt, builtin-commit.c, cache.h, config.c, path.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d1e2120..475d544 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -380,7 +380,8 @@ Common unit suffixes of 'k', 'm', or 'g' are supported.
 core.excludesfile::
 	In addition to '.gitignore' (per-directory) and
 	'.git/info/exclude', git looks into this file for patterns
-	of files which are not meant to be tracked.  See
+	of files which are not meant to be tracked.  "~/" and "~user/"
+	are expanded to the specified user's home directory.  See
 	linkgit:gitignore[5].
 
 core.editor::
@@ -670,6 +671,7 @@ color.ui::
 
 commit.template::
 	Specify a file to use as the template for new commit messages.
+	"~/" and "~user/" are expanded to the specified user's home directory.
 
 diff.autorefreshindex::
 	When using 'git-diff' to compare with work tree
diff --git a/builtin-commit.c b/builtin-commit.c
index d525b89..09d2840 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -999,7 +999,7 @@ static int git_commit_config(const char *k, const char *v, void *cb)
 	struct wt_status *s = cb;
 
 	if (!strcmp(k, "commit.template"))
-		return git_config_string(&template_file, k, v);
+		return git_config_pathname(&template_file, k, v);
 
 	return git_status_config(k, v, s);
 }
diff --git a/cache.h b/cache.h
index 71a731d..42f7cd8 100644
--- a/cache.h
+++ b/cache.h
@@ -645,6 +645,7 @@ int set_shared_perm(const char *path, int mode);
 #define adjust_shared_perm(path) set_shared_perm((path), 0)
 int safe_create_leading_directories(char *path);
 int safe_create_leading_directories_const(const char *path);
+extern char *expand_user_path(const char *path);
 char *enter_repo(char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {
@@ -903,6 +904,7 @@ extern unsigned long git_config_ulong(const char *, const char *);
 extern int git_config_bool_or_int(const char *, const char *, int *);
 extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
+extern int git_config_pathname(const char **, const char *, const char *);
 extern int git_config_set(const char *, const char *);
 extern int git_config_set_multivar(const char *, const char *, const char *, int);
 extern int git_config_rename_section(const char *, const char *);
diff --git a/config.c b/config.c
index c644061..b3d1ff4 100644
--- a/config.c
+++ b/config.c
@@ -351,6 +351,16 @@ 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;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */
@@ -474,7 +484,7 @@ static int git_default_core_config(const char *var, const char *value)
 		return git_config_string(&editor_program, var, value);
 
 	if (!strcmp(var, "core.excludesfile"))
-		return git_config_string(&excludes_file, var, value);
+		return git_config_pathname(&excludes_file, var, value);
 
 	if (!strcmp(var, "core.whitespace")) {
 		if (!value)
diff --git a/path.c b/path.c
index 047fdb0..2470f78 100644
--- a/path.c
+++ b/path.c
@@ -11,6 +11,7 @@
  * which is what it's designed for.
  */
 #include "cache.h"
+#include "strbuf.h"
 
 static char bad_path[] = "/bad-path/";
 
@@ -207,43 +208,44 @@ int validate_headref(const char *path)
 	return -1;
 }
 
-static char *user_path(char *buf, char *path, int sz)
+static struct passwd *getpw_str(const char *username, size_t len)
 {
 	struct passwd *pw;
-	char *slash;
-	int len, baselen;
+	char *username_z = xmalloc(len + 1);
+	memcpy(username_z, username, len);
+	username_z[len] = '\0';
+	pw = getpwnam(username_z);
+	free(username_z);
+	return pw;
+}
 
-	if (!path || path[0] != '~')
-		return NULL;
-	path++;
-	slash = strchr(path, '/');
-	if (path[0] == '/' || !path[0]) {
-		pw = getpwuid(getuid());
-	}
-	else {
-		if (slash) {
-			*slash = 0;
-			pw = getpwnam(path);
-			*slash = '/';
-		}
-		else
-			pw = getpwnam(path);
-	}
-	if (!pw || !pw->pw_dir || sz <= strlen(pw->pw_dir))
-		return NULL;
-	baselen = strlen(pw->pw_dir);
-	memcpy(buf, pw->pw_dir, baselen);
-	while ((1 < baselen) && (buf[baselen-1] == '/')) {
-		buf[baselen-1] = 0;
-		baselen--;
-	}
-	if (slash && slash[1]) {
-		len = strlen(slash);
-		if (sz <= baselen + len)
-			return NULL;
-		memcpy(buf + baselen, slash, len + 1);
+/*
+ * 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;
+	const char *first_slash = strchrnul(path, '/');
+	const char *to_copy = path;
+
+	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;
 	}
-	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 +293,18 @@ 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))) {
+				free(newpath);
 				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;
 		}
-- 
1.6.5.2.152.gbbe9e
Matthieu Moy· Nov 18, 2009, 07:29 UTC · re: Matthieu Moy · lore

[PATCH v3] Expand ~ and ~user in core.excludesfile, commit.template

These config variables are parsed to substitute ~ and ~user with getpw entries.

user_path() refactored into new function expand_user_path(), to allow dynamically allocating the return buffer.

Original patch by Karl Chen, modified by Matthieu Moy, and further amended by Junio C Hamano.

Signed-off-by: Karl Chen <quarl@quarl.org>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---

Since v2, I changed the code to use $HOME instead of the actual home to expand ~.

This way, if you run "HOME=/else/where/ git status", then both .gitconfig and ~/.gitignore will be taken from /else/where.

 Documentation/config.txt |    7 +++-
 builtin-commit.c         |    2 +-
 cache.h                  |    2 +
 config.c                 |   12 ++++++-
 path.c                   |   83 +++++++++++++++++++++++++++------------------
 5 files changed, 69 insertions(+), 37 deletions(-)
Show changes to 5 files +69 −37

Documentation/config.txt, builtin-commit.c, cache.h, config.c, path.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index cb73d75..fb1740c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -380,8 +380,9 @@ Common unit suffixes of 'k', 'm', or 'g' are supported.
 core.excludesfile::
 	In addition to '.gitignore' (per-directory) and
 	'.git/info/exclude', git looks into this file for patterns
-	of files which are not meant to be tracked.  See
-	linkgit:gitignore[5].
+	of files which are not meant to be tracked.  "~/" is expanded
+	to the value of `$HOME` and "~user/" to the specified user's
+	home directory.  See linkgit:gitignore[5].
 
 core.editor::
 	Commands such as `commit` and `tag` that lets you edit
@@ -670,6 +671,8 @@ color.ui::
 
 commit.template::
 	Specify a file to use as the template for new commit messages.
+	"~/" is expanded to the value of `$HOME` and "~user/" to the
+	specified user's home directory.
 
 diff.autorefreshindex::
 	When using 'git-diff' to compare with work tree
diff --git a/builtin-commit.c b/builtin-commit.c
index d525b89..09d2840 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -999,7 +999,7 @@ static int git_commit_config(const char *k, const char *v, void *cb)
 	struct wt_status *s = cb;
 
 	if (!strcmp(k, "commit.template"))
-		return git_config_string(&template_file, k, v);
+		return git_config_pathname(&template_file, k, v);
 
 	return git_status_config(k, v, s);
 }
diff --git a/cache.h b/cache.h
index 71a731d..42f7cd8 100644
--- a/cache.h
+++ b/cache.h
@@ -645,6 +645,7 @@ int set_shared_perm(const char *path, int mode);
 #define adjust_shared_perm(path) set_shared_perm((path), 0)
 int safe_create_leading_directories(char *path);
 int safe_create_leading_directories_const(const char *path);
+extern char *expand_user_path(const char *path);
 char *enter_repo(char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {
@@ -903,6 +904,7 @@ extern unsigned long git_config_ulong(const char *, const char *);
 extern int git_config_bool_or_int(const char *, const char *, int *);
 extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
+extern int git_config_pathname(const char **, const char *, const char *);
 extern int git_config_set(const char *, const char *);
 extern int git_config_set_multivar(const char *, const char *, const char *, int);
 extern int git_config_rename_section(const char *, const char *);
diff --git a/config.c b/config.c
index c644061..b3d1ff4 100644
--- a/config.c
+++ b/config.c
@@ -351,6 +351,16 @@ 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;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */
@@ -474,7 +484,7 @@ static int git_default_core_config(const char *var, const char *value)
 		return git_config_string(&editor_program, var, value);
 
 	if (!strcmp(var, "core.excludesfile"))
-		return git_config_string(&excludes_file, var, value);
+		return git_config_pathname(&excludes_file, var, value);
 
 	if (!strcmp(var, "core.whitespace")) {
 		if (!value)
diff --git a/path.c b/path.c
index 047fdb0..6351b57 100644
--- a/path.c
+++ b/path.c
@@ -11,6 +11,7 @@
  * which is what it's designed for.
  */
 #include "cache.h"
+#include "strbuf.h"
 
 static char bad_path[] = "/bad-path/";
 
@@ -207,43 +208,49 @@ int validate_headref(const char *path)
 	return -1;
 }
 
-static char *user_path(char *buf, char *path, int sz)
+static struct passwd *getpw_str(const char *username, size_t len)
 {
 	struct passwd *pw;
-	char *slash;
-	int len, baselen;
+	char *username_z = xmalloc(len + 1);
+	memcpy(username_z, username, len);
+	username_z[len] = '\0';
+	pw = getpwnam(username_z);
+	free(username_z);
+	return pw;
+}
 
-	if (!path || path[0] != '~')
-		return NULL;
-	path++;
-	slash = strchr(path, '/');
-	if (path[0] == '/' || !path[0]) {
-		pw = getpwuid(getuid());
-	}
-	else {
-		if (slash) {
-			*slash = 0;
-			pw = getpwnam(path);
-			*slash = '/';
+/*
+ * 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;
+	const char *first_slash = strchrnul(path, '/');
+	const char *to_copy = path;
+
+	if (path == NULL)
+		goto return_null;
+	if (path[0] == '~') {
+		const char *username = path + 1;
+		size_t username_len = first_slash - username;
+		if (username_len == 0) {
+			const char * home = getenv("HOME");
+			strbuf_add(&user_path, home, strlen(home));
+		} else {
+			struct passwd *pw = getpw_str(username, username_len);
+			if (!pw)
+				goto return_null;
+			strbuf_add(&user_path, pw->pw_dir, strlen(pw->pw_dir));
 		}
-		else
-			pw = getpwnam(path);
+		to_copy = first_slash;
 	}
-	if (!pw || !pw->pw_dir || sz <= strlen(pw->pw_dir))
-		return NULL;
-	baselen = strlen(pw->pw_dir);
-	memcpy(buf, pw->pw_dir, baselen);
-	while ((1 < baselen) && (buf[baselen-1] == '/')) {
-		buf[baselen-1] = 0;
-		baselen--;
-	}
-	if (slash && slash[1]) {
-		len = strlen(slash);
-		if (sz <= baselen + len)
-			return NULL;
-		memcpy(buf + baselen, slash, len + 1);
-	}
-	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,18 @@ 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))) {
+				free(newpath);
 				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;
 		}
-- 
1.6.5.2.152.gbbe9e
Matthieu Moy· Nov 18, 2009, 08:58 UTC · re: Matthieu Moy · lore

[PATCH v4] Expand ~ and ~user in core.excludesfile, commit.template

These config variables are parsed to substitute ~ and ~user with getpw entries.

user_path() refactored into new function expand_user_path(), to allow dynamically allocating the return buffer.

Original patch by Karl Chen, modified by Matthieu Moy, and further amended by Junio C Hamano.

Signed-off-by: Karl Chen <quarl@quarl.org>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
"Oops, I did it again :-("
Just a style issue since v3 (char * home -> char *home), sorry for the noise.
 Documentation/config.txt |    7 +++-
 builtin-commit.c         |    2 +-
 cache.h                  |    2 +
 config.c                 |   12 ++++++-
 path.c                   |   83 +++++++++++++++++++++++++++------------------
 5 files changed, 69 insertions(+), 37 deletions(-)
Show changes to 5 files +69 −37

Documentation/config.txt, builtin-commit.c, cache.h, config.c, path.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 78ee906..b4fe843 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -380,8 +380,9 @@ Common unit suffixes of 'k', 'm', or 'g' are supported.
 core.excludesfile::
 	In addition to '.gitignore' (per-directory) and
 	'.git/info/exclude', git looks into this file for patterns
-	of files which are not meant to be tracked.  See
-	linkgit:gitignore[5].
+	of files which are not meant to be tracked.  "~/" is expanded
+	to the value of `$HOME` and "~user/" to the specified user's
+	home directory.  See linkgit:gitignore[5].
 
 core.editor::
 	Commands such as `commit` and `tag` that lets you edit
@@ -681,6 +682,8 @@ color.ui::
 
 commit.template::
 	Specify a file to use as the template for new commit messages.
+	"~/" is expanded to the value of `$HOME` and "~user/" to the
+	specified user's home directory.
 
 diff.autorefreshindex::
 	When using 'git-diff' to compare with work tree
diff --git a/builtin-commit.c b/builtin-commit.c
index 0fd7af6..87d1b90 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -1175,7 +1175,7 @@ static int git_commit_config(const char *k, const char *v, void *cb)
 	struct wt_status *s = cb;
 
 	if (!strcmp(k, "commit.template"))
-		return git_config_string(&template_file, k, v);
+		return git_config_pathname(&template_file, k, v);
 
 	return git_status_config(k, v, s);
 }
diff --git a/cache.h b/cache.h
index f7533ff..2ba9690 100644
--- a/cache.h
+++ b/cache.h
@@ -649,6 +649,7 @@ int set_shared_perm(const char *path, int mode);
 #define adjust_shared_perm(path) set_shared_perm((path), 0)
 int safe_create_leading_directories(char *path);
 int safe_create_leading_directories_const(const char *path);
+extern char *expand_user_path(const char *path);
 char *enter_repo(char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {
@@ -909,6 +910,7 @@ extern unsigned long git_config_ulong(const char *, const char *);
 extern int git_config_bool_or_int(const char *, const char *, int *);
 extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
+extern int git_config_pathname(const char **, const char *, const char *);
 extern int git_config_set(const char *, const char *);
 extern int git_config_set_multivar(const char *, const char *, const char *, int);
 extern int git_config_rename_section(const char *, const char *);
diff --git a/config.c b/config.c
index 51f2208..37385ce 100644
--- a/config.c
+++ b/config.c
@@ -351,6 +351,16 @@ 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;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */
@@ -479,7 +489,7 @@ static int git_default_core_config(const char *var, const char *value)
 		return git_config_string(&editor_program, var, value);
 
 	if (!strcmp(var, "core.excludesfile"))
-		return git_config_string(&excludes_file, var, value);
+		return git_config_pathname(&excludes_file, var, value);
 
 	if (!strcmp(var, "core.whitespace")) {
 		if (!value)
diff --git a/path.c b/path.c
index c7679be..2ec950b 100644
--- a/path.c
+++ b/path.c
@@ -11,6 +11,7 @@
  * which is what it's designed for.
  */
 #include "cache.h"
+#include "strbuf.h"
 
 static char bad_path[] = "/bad-path/";
 
@@ -207,43 +208,49 @@ int validate_headref(const char *path)
 	return -1;
 }
 
-static char *user_path(char *buf, char *path, int sz)
+static struct passwd *getpw_str(const char *username, size_t len)
 {
 	struct passwd *pw;
-	char *slash;
-	int len, baselen;
+	char *username_z = xmalloc(len + 1);
+	memcpy(username_z, username, len);
+	username_z[len] = '\0';
+	pw = getpwnam(username_z);
+	free(username_z);
+	return pw;
+}
 
-	if (!path || path[0] != '~')
-		return NULL;
-	path++;
-	slash = strchr(path, '/');
-	if (path[0] == '/' || !path[0]) {
-		pw = getpwuid(getuid());
-	}
-	else {
-		if (slash) {
-			*slash = 0;
-			pw = getpwnam(path);
-			*slash = '/';
+/*
+ * 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;
+	const char *first_slash = strchrnul(path, '/');
+	const char *to_copy = path;
+
+	if (path == NULL)
+		goto return_null;
+	if (path[0] == '~') {
+		const char *username = path + 1;
+		size_t username_len = first_slash - username;
+		if (username_len == 0) {
+			const char *home = getenv("HOME");
+			strbuf_add(&user_path, home, strlen(home));
+		} else {
+			struct passwd *pw = getpw_str(username, username_len);
+			if (!pw)
+				goto return_null;
+			strbuf_add(&user_path, pw->pw_dir, strlen(pw->pw_dir));
 		}
-		else
-			pw = getpwnam(path);
+		to_copy = first_slash;
 	}
-	if (!pw || !pw->pw_dir || sz <= strlen(pw->pw_dir))
-		return NULL;
-	baselen = strlen(pw->pw_dir);
-	memcpy(buf, pw->pw_dir, baselen);
-	while ((1 < baselen) && (buf[baselen-1] == '/')) {
-		buf[baselen-1] = 0;
-		baselen--;
-	}
-	if (slash && slash[1]) {
-		len = strlen(slash);
-		if (sz <= baselen + len)
-			return NULL;
-		memcpy(buf + baselen, slash, len + 1);
-	}
-	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,18 @@ 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))) {
+				free(newpath);
 				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;
 		}
-- 
1.6.5.2.152.gbbe9e
Matthieu Moy· Nov 19, 2009, 15:21 UTC · re: Matthieu Moy · lore

[PATCH] expand_user_path: expand ~ to $HOME, not to the actual homedir.

In 395de250d (Expand ~ and ~user in core.excludesfile, commit.template), we introduced the mechanism. But expanding ~ using getpw is not what people overriding $HOME would usually expect. In particular, git looks for the user's .gitconfig using $HOME, so it's better to be consistent.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
This is basically my patch v4, but since an earlier version is already
in next, I resend the last bits of it, based on next.
 Documentation/config.txt |    9 +++++----
 path.c                   |   13 +++++++++----
 2 files changed, 14 insertions(+), 8 deletions(-)
Show changes to 2 files +14 −8

Documentation/config.txt, path.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 468e285..39d1226 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -381,9 +381,9 @@ Common unit suffixes of 'k', 'm', or 'g' are supported.
 core.excludesfile::
 	In addition to '.gitignore' (per-directory) and
 	'.git/info/exclude', git looks into this file for patterns
-	of files which are not meant to be tracked.  "~/" and "~user/"
-	are expanded to the specified user's home directory.  See
-	linkgit:gitignore[5].
+	of files which are not meant to be tracked.  "~/" is expanded
+	to the value of `$HOME` and "~user/" to the specified user's
+	home directory.  See linkgit:gitignore[5].
 
 core.editor::
 	Commands such as `commit` and `tag` that lets you edit
@@ -683,7 +683,8 @@ color.ui::
 
 commit.template::
 	Specify a file to use as the template for new commit messages.
-	"~/" and "~user/" are expanded to the specified user's home directory.
+	"~/" is expanded to the value of `$HOME` and "~user/" to the
+	specified user's home directory.
 
 diff.autorefreshindex::
 	When using 'git-diff' to compare with work tree
diff --git a/path.c b/path.c
index b4a8075..2ec950b 100644
--- a/path.c
+++ b/path.c
@@ -235,10 +235,15 @@ char *expand_user_path(const char *path)
 	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));
+		if (username_len == 0) {
+			const char *home = getenv("HOME");
+			strbuf_add(&user_path, home, strlen(home));
+		} else {
+			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;
 	}
 	strbuf_add(&user_path, to_copy, strlen(to_copy));
-- 
1.6.5.2.152.gbbe9e
Jeff King· Nov 19, 2009, 15:23 UTC · re: Matthieu Moy · lore

Re: [PATCH] expand_user_path: expand ~ to $HOME, not to the actual homedir.

On Thu, Nov 19, 2009 at 04:21:15PM +0100, Matthieu Moy wrote:
Show 6 quoted lines
> -	of files which are not meant to be tracked.  "~/" and "~user/"
> -	are expanded to the specified user's home directory.  See
> -	linkgit:gitignore[5].
> +	of files which are not meant to be tracked.  "~/" is expanded
> +	to the value of `$HOME` and "~user/" to the specified user's
> +	home directory.  See linkgit:gitignore[5].

Thanks. As a side effect, this wording change addresses my original ambiguity concern (and I also think using $HOME is the right thing to do).

-Peff
Matthieu Moy· Nov 19, 2009, 16:32 UTC · re: Jeff King · lore

Re: [PATCH] expand_user_path: expand ~ to $HOME, not to the actual homedir.

Jeff King <peff@peff.net> writes:
Show 12 quoted lines
> On Thu, Nov 19, 2009 at 04:21:15PM +0100, Matthieu Moy wrote:
>
>> -	of files which are not meant to be tracked.  "~/" and "~user/"
>> -	are expanded to the specified user's home directory.  See
>> -	linkgit:gitignore[5].
>> +	of files which are not meant to be tracked.  "~/" is expanded
>> +	to the value of `$HOME` and "~user/" to the specified user's
>> +	home directory.  See linkgit:gitignore[5].
>
> Thanks. As a side effect, this wording change addresses my original
> ambiguity concern (and I also think using $HOME is the right thing to
> do).

Not a pure coincidence indeed ;-). In this new version, ~ and ~foo do actually different things so they definitely deserve the heaviness of a few extra words.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Nov 19, 2009, 18:12 UTC · re: Matthieu Moy · lore

Re: [PATCH v4] Expand ~ and ~user in core.excludesfile, commit.template

Oops; last night I wrote
    Thanks; v3 was already in next so I'll make an interdiff into a separate
    "fix-up" commit.
but didn't send it out.  Thanks for rerolling the patch.

← back to recent threads