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

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

[Matthieu Moy]

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

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

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

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

[Junio C Hamano]

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

> 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.
         */

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

[Junio C Hamano]

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

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

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

[Matthieu Moy]

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/

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

[Jakub Narebski]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

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

> 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

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

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

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

[Matthieu Moy]

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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Jakub Narebski]

Dnia wtorek 17. listopada 2009 09:57, Matthieu Moy napisał:
> 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

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

[Jakub Narebski]

Dnia wtorek 17. listopada 2009 07:22, Junio C Hamano napisał:
> 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)?

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

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

[Jeff King]

On Mon, Nov 16, 2009 at 11:07:40AM +0100, Matthieu Moy wrote:

> 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

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

[Mike Hommey]

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)

Mike

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

[Andreas Schwab]

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.

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

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

[Junio C Hamano]

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?

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

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

[Andreas Schwab]

Junio C Hamano <gitster@pobox.com> writes:

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

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

[Matthieu Moy]

> 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/

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

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> 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/

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

[Jeff King]

On Tue, Nov 17, 2009 at 09:53:52AM +0100, Matthieu Moy wrote:

> 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

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

[Matthieu Moy]

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

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

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

[Matthieu Moy]

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

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

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

[Matthieu Moy]

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

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

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

[Matthieu Moy]

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

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

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

[Jeff King]

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

-Peff

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

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> 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/

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

[Junio C Hamano]

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.

Re: [PATCH] Support "core.excludesfile = ~/.gitignore"

[Junio C Hamano]

Karl Chen <quarl@cs.berkeley.edu> writes:

>>>>>> On 2008-08-22 14:10 PDT, Junio C Hamano writes:
>
>     Junio> If we were to support relative paths, I think it would
>     Junio> be useful and consistent if a relative path found in
>     Junio> ".git/config" is relative to the work tree root, in
>     Junio> "config" in a bare repository relative to the bare
>     Junio> repository, and in "$HOME/.gitconfig" relative to
>     Junio> $HOME.
>
> Makes sense to support it everywhere.  For .git/config, isn't it
> more consistent for it to be relative to .git?

Consistency and usefulness are different things.  Suppose you want as the
upstream of your project maintain and distribute a mail-alias list in-tree
(say, the file is at the root level, CONTRIBUTORS), and you suggest
contributors to use it when using "commit --author".

Which one do you want to write in your README:

	[user]
        	nicknamelistfile = ../CONTRIBUTORS

or

	[user]
        	nicknamelistfile = CONTRIBUTORS

You have to say the former if it is relative to .git/config.

> So, being new to git development, am I correctly assessing your
> response as "with refinement this can be included in git"?

I do not have fundamental objection to what you are trying to achieve
(i.e. being able to say "relative to $HOME").  I personally think the
approach you took in your patch (i.e. only support "~/" and use $HOME,
without any other fancy stuff) is a sensible first cut for that issue.

I just pointed out possible design issues about the future direction after
that first cut.  When I make comments on design-level issues, I rarely
read the patch itself very carefully, so it is a different issue if your
particular implementation in the patch is the best implementation of that
first cut approach.

Re: [PATCH] Support "core.excludesfile = ~/.gitignore"

[Jeff King]

On Sun, Aug 24, 2008 at 11:11:14AM -0700, Junio C Hamano wrote:

> Consistency and usefulness are different things.  Suppose you want as the
> upstream of your project maintain and distribute a mail-alias list in-tree
> (say, the file is at the root level, CONTRIBUTORS), and you suggest
> contributors to use it when using "commit --author".
> 
> Which one do you want to write in your README:
> 
> 	[user]
>         	nicknamelistfile = ../CONTRIBUTORS
> 
> or
> 
> 	[user]
>         	nicknamelistfile = CONTRIBUTORS
> 
> You have to say the former if it is relative to .git/config.

Couldn't the exact opposite argument be made for "suppose you want to
put the mail-alias file in a repo-specific directory that was not
tracked?" I.e., you are trading off "CONTRIBUTORS" against
".git/CONTRIBUTORS". So which one inconveniences the smallest number of
people is really a question of what people want to do with such pointers
(and since we don't support any yet, we don't really know...).

But more worrisome to me is that the working directory and git directory
do not necessarily follow a "../" and ".git/" relationship. How would
you resolve "../foo" with:

  GIT_DIR=/path/to/other/place git ...

or

  git --work-tree=/path/to/other/place

?

If you want to be able to point to either, I suspect we are better off
simply introducing some basic substitutions like $GIT_DIR and
$GIT_WORK_TREE. Maybe even just allow environment variable expansion,
and then promise to set those variables, which takes care of $HOME
automagically.

-Peff

Re: [PATCH] Support "core.excludesfile = ~/.gitignore"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Aug 24, 2008 at 11:11:14AM -0700, Junio C Hamano wrote:
>
>> Consistency and usefulness are different things.  Suppose you want as the
>> upstream of your project maintain and distribute a mail-alias list in-tree
>> (say, the file is at the root level, CONTRIBUTORS), and you suggest
>> contributors to use it when using "commit --author".
>> 
>> Which one do you want to write in your README:
>> 
>> 	[user]
>>         	nicknamelistfile = ../CONTRIBUTORS
>> 
>> or
>> 
>> 	[user]
>>         	nicknamelistfile = CONTRIBUTORS
>> 
>> You have to say the former if it is relative to .git/config.
>
> Couldn't the exact opposite argument be made for "suppose you want to
> put the mail-alias file in a repo-specific directory that was not
> tracked?" I.e., you are trading off "CONTRIBUTORS" against
> ".git/CONTRIBUTORS".

No, I couldn't ;-)

Why would you write what you wrote in README?

Anything you store in .git is not propagated, so the instruction would not
likely to be "store it in .git/CONTRIBUTORS and point at it".  There is no
merit in forcing users to standardize on "in .git".  The instruction would
be to "store it anywhere you want, and point at it".

The example I gave is very different.  It points at an in-tree thing, and
anybody who has worktree checked out will have it _at the location I as
the README writer expect it to be_.  That is the difference that makes the
exact opposite argument much weaker.

That's why I suggested "relative to work tree if in .git/config, or gitdir
if in config of a bare repository", although honestly speaking I do not
have very strong preference either way.

> If you want to be able to point to either, I suspect we are better off
> simply introducing some basic substitutions like $GIT_DIR and
> $GIT_WORK_TREE. Maybe even just allow environment variable expansion,
> and then promise to set those variables, which takes care of $HOME
> automagically.

Because we haven't deprecated core.worktree (or $GIT_WORK_TREE) yet, your
suggestion has an obvious chicken-and-egg problem, even though otherwise I
think it makes perfect sense and very much like it.

Perhaps we should rid of the worktree that is separate and floats
unrelated to where $GIT_DIR is.

Re: [PATCH] Support "core.excludesfile = ~/.gitignore"

[Jeff King]

On Sun, Aug 24, 2008 at 03:59:49PM -0700, Junio C Hamano wrote:

> > Couldn't the exact opposite argument be made for "suppose you want to
> > put the mail-alias file in a repo-specific directory that was not
> > tracked?" I.e., you are trading off "CONTRIBUTORS" against
> > ".git/CONTRIBUTORS".
> 
> No, I couldn't ;-)
> 
> Why would you write what you wrote in README?
> 
> Anything you store in .git is not propagated, so the instruction would not
> likely to be "store it in .git/CONTRIBUTORS and point at it".  There is no
> merit in forcing users to standardize on "in .git".  The instruction would
> be to "store it anywhere you want, and point at it".

Ah, right.

I still think there is a little bit of convenience when you are doing
something totally personal (i.e., not putting it in a README, but rather
just wanting to store the referenced file in .git for the sake of
simplicity). But in that case, it is only slightly less convenient to
just point to the full path. So your example trumps this, since you have
no sane way of knowing the full path in README instructions.

> Because we haven't deprecated core.worktree (or $GIT_WORK_TREE) yet, your
> suggestion has an obvious chicken-and-egg problem, even though otherwise I
> think it makes perfect sense and very much like it.

You might be able to get around that by lazily filling in the variable.
IOW, expand it at the point-of-use rather than while reading the config.
However, the point of use might easily have something to do with reading
config, so that re-creates the cycle.

In general, I think we treat config as order-independent. We could make
the use of such variables order-dependent (i.e., if you haven't set
core.worktree, then we give you the value without having set it, and we
recalculate later. Confusing results, but at least a simple rule to
understand).

I think there actually are a few other order-dependent things in the
config, like the order of multi-value keys like push and fetch refspecs.

Would you want this expansion only for specially marked variables, or
for all variables? I like the concept of general templates for config
values, but it will backwards compatibility, especially for alias.*.

> Perhaps we should rid of the worktree that is separate and floats
> unrelated to where $GIT_DIR is.

I assumed people were actually using it, which is why it was
implemented.

-Peff

Re: [PATCH] Support "core.excludesfile = ~/.gitignore"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Perhaps we should rid of the worktree that is separate and floats
>> unrelated to where $GIT_DIR is.
>
> I assumed people were actually using it, which is why it was
> implemented.

Judging from the occasional "I tried core.worktree but it does not work in
this and that situations" I see here and on #git, my impression is that
new people try it, saying "git is cool -- unlike cvs that sprinkles those
ugly CVS directories all over the place, it only contaminates my work tree
with a single directory '.git' and nothing else.  Ah, wait --- what's this
core.worktree thing?  Can I get rid of that last one as well?  That sounds
even cooler".

IOW, I do not think it is really _needed_ per-se as a feature, but it was
done because it was thought to be doable, which unfortunately turned out
to involve hair-pulling complexity that the two attempts that led to the
current code still haven't resolved.

I really wish we do not have to worry about that anymore.

[PATCH v2] Support "core.excludesfile = ~/.gitignore"

[Karl Chen]

The config variable core.excludesfile is parsed to substitute ~ and ~user with
getpw entries.

Signed-off-by: Karl Chen <quarl@quarl.org>
---
 config.c |   41 +++++++++++++++++++++++++++++++++++++++--
 1 files changed, 39 insertions(+), 2 deletions(-)


Based on the discussion it sounds like there are complications to
supporting relative paths (due to worktree config), and "$HOME"
(when generalized, due to bootstrapping issues with $GIT_*).

Since ~ and ~user are orthogonal to these, can I suggest going
forward with this, without blocking on those two?

I have reworked the patch to use getpw to support ~user.  $HOME
can eventually be supported via $ENVVARs.


diff --git a/config.c b/config.c
index 53f04a0..6a83c64 100644
--- a/config.c
+++ b/config.c
@@ -334,6 +334,42 @@ int git_config_string(const char **dest, const char *var, const char *value)
 	return 0;
 }
 
+/*
+ * Expand ~ and ~user.  Returns a newly malloced string.  (If input does not
+ * start with "~", equivalent to xstrdup.)
+ */
+static char *expand_userdir(const char *value) {
+	if (value[0] == '~') {
+		struct passwd *pw;
+		char *expanded_dir;
+		const char *slash = strchr(value+1, '/');
+		const char *after_username = slash ? slash : value+strlen(value);
+		if (after_username == value+1) {
+			pw = getpwuid(getuid());
+			if (!pw) die("You don't exist!");
+		} else {
+			char save = *after_username;
+			*(char*)after_username = '\0';
+			pw = getpwnam(value+1);
+			if (!pw) die("No such user: '%s'", value+1);
+			*(char*)after_username = save;
+		}
+		expanded_dir = xmalloc(strlen(pw->pw_dir) + strlen(after_username) + 1);
+		strcpy(expanded_dir, pw->pw_dir);
+		strcat(expanded_dir, after_username);
+		return expanded_dir;
+	} else {
+		return xstrdup(value);
+	}
+}
+
+int git_config_userdir(const char **dest, const char *var, const char *value) {
+	if (!value)
+		return config_error_nonbool(var);
+	*dest = expand_userdir(value);
+	return 0;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */
@@ -456,8 +492,9 @@ static int git_default_core_config(const char *var, const char *value)
 	if (!strcmp(var, "core.editor"))
 		return git_config_string(&editor_program, var, value);
 
-	if (!strcmp(var, "core.excludesfile"))
-		return git_config_string(&excludes_file, var, value);
+	if (!strcmp(var, "core.excludesfile")) {
+		return git_config_userdir(&excludes_file, var, value);
+	}
 
 	if (!strcmp(var, "core.whitespace")) {
 		if (!value)
-- 
1.5.6.2

Re: [PATCH v2] Support "core.excludesfile = ~/.gitignore"

[Jeff King]

On Mon, Aug 25, 2008 at 12:07:15PM -0700, Karl Chen wrote:

> Based on the discussion it sounds like there are complications to
> supporting relative paths (due to worktree config), and "$HOME"
> (when generalized, due to bootstrapping issues with $GIT_*).

I think that is fine for now. One other simple possibility would be to
expand _just_ $HOME, and then if we later decided to do all environment
variables it would naturally encompass that. However, we might want to
support "~" then anyway, so I think doing "~" first is fine.

However, there are two problems with the patch:

  1. It should probably re-use path.c:user_path, as Johannes mentioned.

  2. There is no documentation update.

Also, are there any other config variables which would benefit from this
substitution (I can't think of any off-hand, but there are quite a few I
don't use).

-Peff

Re: [PATCH v2] Support "core.excludesfile = ~/.gitignore"

[Karl Chen]

>>>>> On 2008-08-26 17:25 PDT, Jeff King writes:

    Jeff>   1. It should probably re-use path.c:user_path, as
    Jeff>      Johannes mentioned.

That function has the wrong interface for this task (requires
extra strdup, imposes PATH_MAX, conflates all error conditions
into returning NULL, also returns NULL if input doesn't have "~").
Do you still think it should re-use that function?

Re: [PATCH v2] Support "core.excludesfile = ~/.gitignore"

[Junio C Hamano]

Karl Chen <quarl@cs.berkeley.edu> writes:

>>>>>> On 2008-08-26 17:25 PDT, Jeff King writes:
>
>     Jeff>   1. It should probably re-use path.c:user_path, as
>     Jeff>      Johannes mentioned.
>
> That function has the wrong interface for this task (requires
> extra strdup, imposes PATH_MAX, conflates all error conditions
> into returning NULL, also returns NULL if input doesn't have "~").
> Do you still think it should re-use that function?

On the other hand you have a pair of ugly "casting a const pointer down,
temporarily modifying what is const" in your version.

In any case, I think their point is that these two functions are meant to
do the same thing, and a unified interface would be desirable.

You do not necessarily have to use user_path() from path.c, but
user_path(), if its interface is coarser, could become a thin wrapper
around yours, don't you think?  Even better, perhaps the current callers
of user_path() may benefit from finer distinction among error conditions
(I didn't look, though).

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

[Karl Chen]

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.

Signed-off-by: Karl Chen <quarl@quarl.org>
---

>>>>> On 2008-08-26 22:01 PDT, Junio C Hamano writes:

>>>>> On 2008-08-26 17:25 PDT, Jeff King writes:
    >> 
    Jeff> 1. It should probably re-use path.c:user_path, as
    Jeff> Johannes mentioned.
    >> 
    >> That function has the wrong interface for this task
    >> (requires extra strdup, imposes PATH_MAX, conflates all
    >> error conditions into returning NULL, also returns NULL if
    >> input doesn't have "~").  Do you still think it should
    >> re-use that function?

    Junio> On the other hand you have a pair of ugly "casting a
    Junio> const pointer down, temporarily modifying what is
    Junio> const" in your version.

user_path() does the same thing; it's just obscured by the lack of
type qualifiers.  I rationalized it because git has much bigger
problems if threading support were added.  But I agree it's ugly.
The new patch avoids it.

    Junio> In any case, I think their point is that these two
    Junio> functions are meant to do the same thing, and a unified
    Junio> interface would be desirable.

There is a lot of refactoring I would do were I maintaining the
code.  I figured a minimally invasive change would be more easily
accepted.  Anyway here is a patch to unify "~" expansion.

 builtin-commit.c |    2 +-
 cache.h          |    2 +
 config.c         |   13 +++++++-
 path.c           |   88 +++++++++++++++++++++++++++++++++--------------------
 4 files changed, 69 insertions(+), 36 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 649c8be..e510207 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -891,7 +891,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 static int git_commit_config(const char *k, const char *v, void *cb)
 {
 	if (!strcmp(k, "commit.template"))
-		return git_config_string(&template_file, k, v);
+		return git_config_userdir(&template_file, k, v);
 
 	return git_status_config(k, v, cb);
 }
diff --git a/cache.h b/cache.h
index ab9f97e..096fd9d 100644
--- a/cache.h
+++ b/cache.h
@@ -527,6 +527,7 @@ int git_config_perm(const char *var, const char *value);
 int adjust_shared_perm(const char *path);
 int safe_create_leading_directories(char *path);
 int safe_create_leading_directories_const(const char *path);
+extern char *expand_user_path(char *buf, const char *path, int sz);
 char *enter_repo(char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {
@@ -748,6 +749,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_userdir(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 53f04a0..3395283 100644
--- a/config.c
+++ b/config.c
@@ -334,6 +334,14 @@ int git_config_string(const char **dest, const char *var, const char *value)
 	return 0;
 }
 
+int git_config_userdir(const char **dest, const char *var, const char *value) {
+	if (!value)
+		return config_error_nonbool(var);
+	*dest = expand_user_path(NULL, value, 0);
+	if (!*dest || !**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 */
@@ -456,8 +464,9 @@ static int git_default_core_config(const char *var, const char *value)
 	if (!strcmp(var, "core.editor"))
 		return git_config_string(&editor_program, var, value);
 
-	if (!strcmp(var, "core.excludesfile"))
-		return git_config_string(&excludes_file, var, value);
+	if (!strcmp(var, "core.excludesfile")) {
+		return git_config_userdir(&excludes_file, var, value);
+	}
 
 	if (!strcmp(var, "core.whitespace")) {
 		if (!value)
diff --git a/path.c b/path.c
index 76e8872..ef6dbaa 100644
--- a/path.c
+++ b/path.c
@@ -137,43 +137,65 @@ int validate_headref(const char *path)
 	return -1;
 }
 
-static char *user_path(char *buf, char *path, int sz)
+static inline struct passwd *getpw_strspan(const char *begin_username,
+										   const char *end_username)
 {
-	struct passwd *pw;
-	char *slash;
-	int len, baselen;
-
-	if (!path || path[0] != '~')
-		return NULL;
-	path++;
-	slash = strchr(path, '/');
-	if (path[0] == '/' || !path[0]) {
-		pw = getpwuid(getuid());
+	if (begin_username == end_username) {
+		return getpwuid(getuid());
+	} else {
+		size_t username_len = end_username - begin_username;
+		char *username = alloca(username_len + 1);
+		memcpy(username, begin_username, username_len);
+		username[username_len] = '\0';
+		return getpwnam(username);
 	}
-	else {
-		if (slash) {
-			*slash = 0;
-			pw = getpwnam(path);
-			*slash = '/';
-		}
-		else
-			pw = getpwnam(path);
+}
+
+static inline char *concatstr(char *buf, const char *str1, const char *str2,
+							  size_t bufsz)
+{
+	size_t len1 = strlen(str1);
+	size_t len2 = strlen(str2);
+	size_t needbuflen = len1 + len2 + 1;
+	if (buf) {
+		if (needbuflen > bufsz) return NULL;
+	} else {
+		buf = xmalloc(needbuflen);
 	}
-	if (!pw || !pw->pw_dir || sz <= strlen(pw->pw_dir))
+	memcpy(buf, str1, len1);
+	memcpy(buf+len1, str2, len2+1);
+	return buf;
+}
+
+static inline const char *strchr_or_end(const char *str, char c)
+{
+	while (*str && *str != c) ++str;
+	return str;
+}
+
+/*
+ * Return a string with ~ and ~user expanded via getpw*.  If buf != NULL, then
+ * it is the output buffer with size sz (including terminator); else the
+ * return buffer is xmalloced.  Returns NULL on getpw failure or if the input
+ * buffer is too small.
+ */
+char *expand_user_path(char *buf, const char *path, int sz)
+{
+	if (path == NULL) {
 		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);
+	} else if (path[0] != '~') {
+		if (buf == NULL) {
+			return xstrdup(path);
+		} else {
+			if (strlen(path)+1 > sz) return NULL;
+			return strcpy(buf, path);
+		}
+	} else {
+		const char *after_username = strchr_or_end(path+1, '/');
+		struct passwd *pw = getpw_strspan(path+1, after_username);
+		if (!pw) return NULL;
+		return concatstr(buf, pw->pw_dir, after_username, sz);
 	}
-	return buf;
 }
 
 /*
@@ -221,7 +243,7 @@ char *enter_repo(char *path, int strict)
 		if (PATH_MAX <= len)
 			return NULL;
 		if (path[0] == '~') {
-			if (!user_path(used_path, path, PATH_MAX))
+			if (!expand_user_path(used_path, path, PATH_MAX))
 				return NULL;
 			strcpy(validated_path, path);
 			path = used_path;
-- 
1.5.6.3

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

[Jeff King]

On Thu, Aug 28, 2008 at 02:09:38AM -0700, Karl Chen wrote:

>  builtin-commit.c |    2 +-
>  cache.h          |    2 +
>  config.c         |   13 +++++++-
>  path.c           |   88 +++++++++++++++++++++++++++++++++--------------------
>  4 files changed, 69 insertions(+), 36 deletions(-)

Documentation?

>  	if (!strcmp(k, "commit.template"))
> -		return git_config_string(&template_file, k, v);
> +		return git_config_userdir(&template_file, k, v);

I like this.

> +int git_config_userdir(const char **dest, const char *var, const char *value) {
> +	if (!value)
> +		return config_error_nonbool(var);
> +	*dest = expand_user_path(NULL, value, 0);
> +	if (!*dest || !**dest) die("Failed to expand user dir in: '%s'", value);
> +	return 0;
> +}

I am not sure about !**dest here. This precludes somebody from using "".
While it might not matter here, if there are other users of
git_config_userdir(), they might want to allow a blank entry.

Also, style: there should be a newline after conditional but before
executed code. IOW, replace

  if (cond) code;

with

  if (cond)
          code;

> +static inline struct passwd *getpw_strspan(const char *begin_username,
> +										   const char *end_username) 

1. There seem to be extra tabs in the second line, pushing the
   end_username argument way too far to the right.

2. I'm not sure "strspan" is a good name for this helper, since it calls
   to mind the strspn C function, which is not really related to this at
   all.

3. Usually helper functions that take a non-terminated string like this
   in git use the combination of (char *begin, int len) instead of two
   pointers. While you are currently the only user of the helper, I
   think it makes sense to follow that convention for future users.

> +	if (begin_username == end_username) {
> +		return getpwuid(getuid());
> +	} else {

Style: omit braces on one-liner conditionals:

  if (begin_username == end_username)
          return getwpuid(getuid());

Also, you do a lot of early returns in your code. I think this is good,
because it makes it more readable. But that means you don't have to
worry about "else"ing the other half of the conditional, because you
have already returned. Which makes it even easier to read.

> +		size_t username_len = end_username - begin_username;

See, here you end up converting back from two pointers to a pointer and
a length. Which is why I think we tend to use the other representation.

> +		char *username = alloca(username_len + 1);

I don't think we use alloca() anywhere else. I don't know if there are
portability issues.

> +static inline char *concatstr(char *buf, const char *str1, const char *str2,
> +							  size_t bufsz)
> +{
> +	size_t len1 = strlen(str1);
> +	size_t len2 = strlen(str2);
> +	size_t needbuflen = len1 + len2 + 1;
> +	if (buf) {
> +		if (needbuflen > bufsz) return NULL;
> +	} else {
> +		buf = xmalloc(needbuflen);

Style: more braces which can be omitted.

This function seems a little superfluous, since its semantics are so
specific to this usage. I am all for splitting into little functions,
but I think it would be quite confusing for somebody to try reusing
this. Perhaps it at least needs a comment explaining the semantics of
buf?

> +static inline const char *strchr_or_end(const char *str, char c)
> +{
> +	while (*str && *str != c) ++str;
> +	return str;
> +}

This really seems like premature optimization to me. The only advantage
this has over

  p = strchr(s);
  if (!p)
    p = s + strlen(s);

is that we avoid traversing the string once. But balance that against an
assembler-optimized strchr provided by your libc. And then wonder if it
is even worth it, since this is not even remotely a critical path.

> +{
> +	if (path == NULL) {
>  		return NULL;
> [...]
> +	} else if (path[0] != '~') {
> +		if (buf == NULL) {
> +			return xstrdup(path);
> +		} else {
> +			if (strlen(path)+1 > sz) return NULL;
> +			return strcpy(buf, path);
> +		}

More early returns which can be removed from conditionals.

Also, some of this code seems duplicated with concatstr. Wouldn't it
just be simpler to let concatstr take a NULL for one of the arguments,
and then just use it again here? IOW, something like:

  if (!path)
    return NULL;
  if (path[0] != '~')
    return concatstr(path, NULL);

> -			if (!user_path(used_path, path, PATH_MAX))
> +			if (!expand_user_path(used_path, path, PATH_MAX))

But these functions don't have the same semantics, do they? user_path
used to return NULL if the path didn't start with ~, right?

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Aug 28, 2008 at 02:09:38AM -0700, Karl Chen wrote:
>
>>  builtin-commit.c |    2 +-
>>  cache.h          |    2 +
>>  config.c         |   13 +++++++-
>>  path.c           |   88 +++++++++++++++++++++++++++++++++--------------------
>>  4 files changed, 69 insertions(+), 36 deletions(-)
>
> Documentation?
>
>>  	if (!strcmp(k, "commit.template"))
>> -		return git_config_string(&template_file, k, v);
>> +		return git_config_userdir(&template_file, k, v);
>
> I like this.

Likewise, except for the name.  It is more like "pathname"; "userdir" is
stressing only one aspect of the magic we would do to a value that is a
pathname compared to a value that is a string without any magic.

>> +int git_config_userdir(const char **dest, const char *var, const char *value) {
>> +	if (!value)
>> +		return config_error_nonbool(var);
>> +	*dest = expand_user_path(NULL, value, 0);
>> +	if (!*dest || !**dest) die("Failed to expand user dir in: '%s'", value);
>> +	return 0;
>> +}
>
> I am not sure about !**dest here. This precludes somebody from using "".
> While it might not matter here, if there are other users of
> git_config_userdir(), they might want to allow a blank entry.

True again.

>> +	if (begin_username == end_username) {
>> +		return getpwuid(getuid());
>> +	} else {
>
> Style: omit braces on one-liner conditionals:

... except in cases like this where "else" side is a multi-statement
block, in which case the above is fine.  But as you pointed out, the early
return from here makes the else block unnecessary so you do not need the
braces around "if" side.

>> +		char *username = alloca(username_len + 1);
>
> I don't think we use alloca() anywhere else. I don't know if there are
> portability issues.

Avoidance of alloca() and c99 dynamic array on stack is deliberate in the
current codebase.  Portable use of alloca() is quite hard to get right.

>> +static inline const char *strchr_or_end(const char *str, char c)
>> +{
>> +	while (*str && *str != c) ++str;
>> +	return str;
>> +}
>
> This really seems like premature optimization to me.

So is overuse of inline, it seems.

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

[Karl Chen]

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.

Signed-off-by: Karl Chen <quarl@quarl.org>
---

>>>>> On 2008-08-28 20:26 PDT, Jeff King writes:

    Peff> Documentation?

Documentation added.

    Peff> I am not sure about !**dest here. This precludes
    Peff> somebody from using "".  While it might not matter here,
    Peff> if there are other users of git_config_userdir(), they
    Peff> might want to allow a blank entry.

Point taken.

    Peff> 1. There seem to be extra tabs in the second line,
    Peff>    pushing the end_username argument way too far to the
    Peff>    right.

I had assumed that you guys use tab-width 4 instead of
c-basic-offset 8.  This is one of the reasons I never use tabs
when I can help it.

    Peff> 2. I'm not sure "strspan" is a good name for this
    Peff>    helper, since it calls to mind the strspn C function,
    Peff>    which is not really related to this at all.

Changed to getpw_str().

    Peff> 3. Usually helper functions that take a non-terminated
    Peff>    string like this in git use the combination of (char
    Peff>    *begin, int len) instead of two pointers. While you
    Peff>    are currently the only user of the helper, I think it
    Peff>    makes sense to follow that convention for future
    Peff>    users.

Point taken, changed as suggested.

    Peff> Also, you do a lot of early returns in your code. I
    Peff> think this is good, because it makes it more
    Peff> readable. But that means you don't have to worry about
    Peff> "else"ing the other half of the conditional, because you
    Peff> have already returned. Which makes it even easier to
    Peff> read.

Personally I think early returns without "else"ing is appropriate
for error conditions but not when it's a "do this" or "do that"
switch.  (I wonder if indenting 8 spaces has a long-term effect on
things like this?)  Anyway, I accept the color you picked for this
bikeshed.

    Peff> This function seems a little superfluous, since its
    Peff> semantics are so specific to this usage. I am all for
    Peff> splitting into little functions, but I think it would be
    Peff> quite confusing for somebody to try reusing
    Peff> this. Perhaps it at least needs a comment explaining the
    Peff> semantics of buf?

Comment added.

    Peff> Also, some of this code seems duplicated with
    Peff> concatstr. Wouldn't it just be simpler to let concatstr
    Peff> take a NULL for one of the arguments, and then just use
    Peff> it again here? IOW, something like:

    Peff>   if (!path)
    Peff>     return NULL;
    Peff>   if (path[0] != '~')
    Peff>     return concatstr(path, NULL);

Refactored as suggested.

    >> - if (!user_path(used_path, path, PATH_MAX))
    >> + if (!expand_user_path(used_path, path, PATH_MAX))

    Peff> But these functions don't have the same semantics, do
    Peff> they? user_path used to return NULL if the path didn't
    Peff> start with ~, right?

Yes, but user_path was only called when the input starts with ~.

>>>>> On 2008-08-28 21:08 PDT, Junio C Hamano writes:
    >>> + return git_config_userdir(&template_file, k, v);

    Junio> It is more like "pathname"; "userdir" is stressing only
    Junio> one aspect of the magic we would do to a value that is
    Junio> a pathname compared to a value that is a string without
    Junio> any magic.

Renamed as suggested.

    Junio> Avoidance of alloca() and c99 dynamic array on stack is
    Junio> deliberate in the current codebase.  Portable use of
    Junio> alloca() is quite hard to get right.

Alloca replaced with xmalloc+free.

>>>>> On 2008-08-29 00:00 PDT, Johannes Sixt writes:

    Hannes> Use strchrnul() instead of a home-grown
    Hannes> strchr_or_end().

Didn't know about strchrnul, thanks.

    Hannes> You really should use the strbuf API here. Look for
    Hannes> strbuf_detach() in the existing code.

Unfortunately expand_user_path() needs to support both a fixed
buffer and mallocing return.  I don't think the strbuf API can do
that easily?


 Documentation/config.txt |    4 ++-
 builtin-commit.c         |    2 +-
 cache.h                  |    2 +
 config.c                 |   11 +++++-
 path.c                   |   86 +++++++++++++++++++++++++++++-----------------
 5 files changed, 70 insertions(+), 35 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index af57d94..05e846d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -346,7 +346,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::
@@ -554,6 +555,7 @@ color.status.<slot>::
 
 commit.template::
 	Specify a file to use as the template for new commit messages.
+	"~" and "~user" are expanded to the user's home directory.
 
 color.ui::
 	When set to `always`, always use colors in all git commands which
diff --git a/builtin-commit.c b/builtin-commit.c
index 649c8be..905ebde 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -891,7 +891,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 static int git_commit_config(const char *k, const char *v, void *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, cb);
 }
diff --git a/cache.h b/cache.h
index ab9f97e..3e04794 100644
--- a/cache.h
+++ b/cache.h
@@ -527,6 +527,7 @@ int git_config_perm(const char *var, const char *value);
 int adjust_shared_perm(const char *path);
 int safe_create_leading_directories(char *path);
 int safe_create_leading_directories_const(const char *path);
+extern char *expand_user_path(char *buf, const char *path, int sz);
 char *enter_repo(char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {
@@ -748,6 +749,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 53f04a0..55353d9 100644
--- a/config.c
+++ b/config.c
@@ -334,6 +334,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(NULL, value, 0);
+	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 */
@@ -457,7 +466,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 76e8872..016d072 100644
--- a/path.c
+++ b/path.c
@@ -137,46 +137,68 @@ 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 a string with input strings concatenated.  If buf != NULL, then
+ * it is the output buffer with size bufsz (including terminator); else the
+ * return buffer is xmalloced.  Second string can be NULL, in which case the
+ * first string is simply strduped/strcpyed.  Returns NULL if bufsz is too
+ * small.
+ */
+static inline char *concatstr(char *buf, const char *str1, const char *str2,
+			      size_t bufsz)
+{
+	size_t len1 = strlen(str1);
+	size_t len2 = str ? strlen(str2) : 0;
+	size_t needbuflen = len1 + len2 + 1;
+	if (buf) {
+		if (needbuflen > bufsz)
 			return NULL;
-		memcpy(buf + baselen, slash, len + 1);
+	} else {
+		buf = xmalloc(needbuflen);
 	}
+	memcpy(buf, str1, len1);
+	if (str2)
+		memcpy(buf+len1, str2, len2+1);
 	return buf;
 }
 
 /*
+ * Return a string with ~ and ~user expanded via getpw*.  If buf != NULL, then
+ * it is the output buffer with size bufsz (including terminator); else the
+ * return buffer is xmalloced.  Returns NULL on getpw failure or if the input
+ * buffer is too small.
+ */
+char *expand_user_path(char *buf, const char *path, int bufsz)
+{
+	if (path == NULL)
+		return NULL;
+
+	if (path[0] != '~')
+		return concatstr(buf, path, NULL, bufsz);
+
+	const char *username = path + 1;
+	size_t username_len = strchrnul(username, '/') - username;
+	struct passwd *pw = getpw_str(username, username_len);
+	if (!pw)
+		return NULL;
+	return concatstr(buf, pw->pw_dir, username+username_len, bufsz);
+}
+
+/*
  * First, one directory to try is determined by the following algorithm.
  *
  * (0) If "strict" is given, the path is used as given and no DWIM is
@@ -221,7 +243,7 @@ char *enter_repo(char *path, int strict)
 		if (PATH_MAX <= len)
 			return NULL;
 		if (path[0] == '~') {
-			if (!user_path(used_path, path, PATH_MAX))
+			if (!expand_user_path(used_path, path, PATH_MAX))
 				return NULL;
 			strcpy(validated_path, path);
 			path = used_path;
-- 
1.5.6.3

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

[Junio C Hamano]

Karl Chen <quarl@cs.berkeley.edu> writes:

> 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.
>
> Signed-off-by: Karl Chen <quarl@quarl.org>

Thanks.

> ... Anyway, I accept the color you picked for this
> bikeshed.

I do not think Documentation/CodingStyle is bikesheding but just behaving
like Romans do while in Rome, so that the end result will blend in better.

>     Hannes> You really should use the strbuf API here. Look for
>     Hannes> strbuf_detach() in the existing code.
>
> Unfortunately expand_user_path() needs to support both a fixed
> buffer and mallocing return.  I don't think the strbuf API can do
> that easily?

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

 path.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git i/path.c w/path.c
index 76e8872..c5b253c 100644
--- 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);
+			if (!newpath || (PATH_MAX <= strlen(newpath))) {
+				if (path != newpath)
+					free(newpath);
 				return NULL;
+			}
 			strcpy(validated_path, path);
-			path = used_path;
-		}
-		else if (PATH_MAX - 10 < len)
-			return NULL;
-		else {
+			path = newpath;
+		} else {
 			path = strcpy(used_path, path);
 			strcpy(validated_path, path);
 		}
 		len = strlen(path);
 		for (i = 0; suffix[i]; i++) {
+			if (PATH_MAX <= strlen(suffix[i] + len))
+				continue;
 			strcpy(path + len, suffix[i]);
 			if (!access(path, F_OK)) {
 				strcat(validated_path, suffix[i]);

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

[Karl Chen]

>>>>> On 2008-08-29 09:08 PDT, Junio C Hamano writes:

    Junio> I do not see any strong reason why the single caller of
    Junio> user_path() has to keep using the static allocation.
    Junio> Would it help to reduce the complexity of your
    Junio> expand_user_path() implementation, if we fixed the
    Junio> caller along the lines of this patch (untested, but
    Junio> just to illustrate the point)?

Yes, expand_user_path() would be much simpler, it would basically
be me original implementation except for returning NULL on error.

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

[Junio C Hamano]

Karl Chen <quarl@cs.berkeley.edu> writes:

>>>>>> On 2008-08-29 09:08 PDT, Junio C Hamano writes:
>
>     Junio> I do not see any strong reason why the single caller of
>     Junio> user_path() has to keep using the static allocation.
>     Junio> Would it help to reduce the complexity of your
>     Junio> expand_user_path() implementation, if we fixed the
>     Junio> caller along the lines of this patch (untested, but
>     Junio> just to illustrate the point)?
>
> Yes, expand_user_path() would be much simpler, it would basically
> be me original implementation except for returning NULL on error.

Yeah, modulo those styles issues your v3 and v4 addressed, and use of
strbuf.

It might feel that we went full circles, wasting your time.  But it's not.
We found out that the final series would look like this:

 [1/3] Introduce expand_user_path();

 [2/3] Using #1, introduce git_config_pathname() and use it to parse your
       two variables;

 [3/3] Update the sole caller of user_path() to use expand_user_path().

Patch #1 and #2 can be squashed into one if you want.  Also you do not
have to do #3 yourself if you do not feel like it (but now we know how the
code would look like, why not?).

Thanks to these three initial rounds, we know whoever implements #1 knows
what kind of interface the (to-be-rewritten) user of user_path() would
want, so #3 will become much cleaner.  We made progress.

Thanks.

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

[Karl Chen]

>>>>> On 2008-08-29 12:28 PDT, Junio C Hamano writes:

    Junio>  [3/3] Update the sole caller of user_path() to use
    Junio>  expand_user_path().

Actually I just looked closer at enter_repo() and it's not quite
as simple as your proposed patch, because enter_repo() wants to
concatenate suffixes like ".git".  So either the malloced string
would have to be copied to the static buffer again, or return a
strbuf, or take an argument for allocating extra chars.

Wow, I'd forgotten how much work it is to do string manipulation
in C.

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

[Junio C Hamano]

Karl Chen <quarl@cs.berkeley.edu> writes:

>>>>>> On 2008-08-29 12:28 PDT, Junio C Hamano writes:
>
>     Junio>  [3/3] Update the sole caller of user_path() to use
>     Junio>  expand_user_path().
>
> Actually I just looked closer at enter_repo() and it's not quite
> as simple as your proposed patch, because enter_repo() wants to
> concatenate suffixes like ".git".

I thought the "just an illustration" patch at least took care of that
part.

The thing is that enter_repo() is not performance critical, it is where
server side programs validate the repository path received from the other
end of the network, and we can be stricter than necessary about path
lengths.  I do not particularly think it is necessary to convert it and
use strbuf to allow arbitrarily long paths --- rejecting requests to an
absurdly deep directory is not an end of the world there.

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

[Jeff King]

On Fri, Aug 29, 2008 at 02:29:00AM -0700, Karl Chen wrote:

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

How about:

  A leading "~" or "~user" is expanded to the home directory of the
  current user or "user", as in the shell.

Specifically:

  1. We obviously handle only leading cases, so /path/to/~file~with~tildes
     is ok.

  2. It was a little unclear to me whether both "~" and "~user" expande
     to the same thing. I.e., can one use this for arbitrary "user" (and
     the answer of course is yes).

> +char *expand_user_path(char *buf, const char *path, int bufsz)
> +{
> +	if (path == NULL)
> +		return NULL;
> +
> +	if (path[0] != '~')
> +		return concatstr(buf, path, NULL, bufsz);
> +
> +	const char *username = path + 1;
> +	size_t username_len = strchrnul(username, '/') - username;
> +	struct passwd *pw = getpw_str(username, username_len);

Declaration after statement (we try to remain portable to non-C99
systems).

Other than that, I think the patch is fine (though I am not opposed to
the improvements Junio has mentioned).

-Peff