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

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

[Karl Chen]

I keep my rc files, including .gitconfig and my default gitignore
list under version control and like to have the same contents
everywhere.  Unfortunately my home directory is at different
locations on different systems.

I'd like to be able to put something like this in my ~/.gitconfig:

[core]
        excludesfile = ~/.gitignore

or
        excludesfile = $HOME/.gitignore

Another idea is to have a non-absolute path be interpreted
relative to the location of .gitconfig, i.e. $HOME, instead of the
current directory.  $GIT_DIR/info/excludes is already for
repository-specific excludes so no functionality would be lost.


Below is a sample patch that works for me.  We could also use
getpwuid(getuid()) instead of getenv("HOME") to be consistent with
user_path() but this is simpler and arguably more likely what the
user wants when it matters.


>From 6eb18f8ade791521bdad955e1da2b40399a426f0 Mon Sep 17 00:00:00 2001
From: Karl Chen <quarl@quarl.org>
Date: Thu, 21 Aug 2008 21:00:26 -0700
Subject: [PATCH] Support "core.excludesfile = ~/.gitignore"

The config variable core.excludesfile is parsed to substitute leading "~/"
with getenv("HOME").

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

---
 config.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 53f04a0..41061d2 100644
--- a/config.c
+++ b/config.c
@@ -334,6 +334,18 @@ int git_config_string(const char **dest, const char *var, const char *value)
 	return 0;
 }
 
+static char const *git_config_subst_userdir(char const *value) {
+	if (value[0] == '~' && value[1] == '/') {
+		const char *home = getenv("HOME");
+		char *userdir_excludes_file = malloc(strlen(home) + strlen(value)-1 + 1);
+		strcpy(userdir_excludes_file, home);
+		strcat(userdir_excludes_file, value+1);
+		return userdir_excludes_file;
+	} else {
+		return xstrdup(value);
+	}
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */
@@ -456,8 +468,12 @@ 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")) {
+		if (!value)
+			return config_error_nonbool(var);
+		excludes_file = git_config_subst_userdir(value);
+		return 0;
+	}
 
 	if (!strcmp(var, "core.whitespace")) {
 		if (!value)
-- 
1.5.6.2

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

[Eric Raible]

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

> +static char const *git_config_subst_userdir(char const *value) {
> +	if (value[0] == '~' && value[1] == '/') {

Might you want to check that strlen(value) is at least 2?

- Eric

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

[Bert Wesarg]

On Fri, Aug 22, 2008 at 18:58, Eric Raible <raible@gmail.com> wrote:
> Karl Chen <quarl <at> cs.berkeley.edu> writes:
>
>> +static char const *git_config_subst_userdir(char const *value) {
>> +     if (value[0] == '~' && value[1] == '/') {
>
> Might you want to check that strlen(value) is at least 2?
No.

    swtich (strlen(value)) {
    case 0:
        /* value[0] == '\0' => value[0] != '~'
           value[1] will never be dereferenced, because of lazy && */
    case 1:
       /* value[0] != '\0'
          if (value[0] == '~')
              value[1] == '\0' => value[1] != '/' */
    default:
       /* ... */
    }

So no invalid memory dereferences.

Regards
Bert
>
> - Eric
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Junio C Hamano]

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

> Another idea is to have a non-absolute path be interpreted
> relative to the location of .gitconfig.

If we were to support relative paths, I think it would be useful and
consistent if a relative path found in ".git/config" is relative to the
work tree root, in "config" in a bare repository relative to the bare
repository, and in "$HOME/.gitconfig" relative to $HOME.  I am not sure
what a relative path in "/etc/gitconfig" should be relative to, though.

However, this has a technical difficulty.  When configuration values are
read, the code that knows what the value means does not in general know
which configuration file is being read from.

> Below is a sample patch that works for me.  We could also use
> getpwuid(getuid()) instead of getenv("HOME") to be consistent with
> user_path() but this is simpler and arguably more likely what the
> user wants when it matters.

It is quite likely that somebody would want you to interpret "~name/" if
you advertize that you support "~/", so you would need to call getpwuid()
eventually if you go down this path.  I wonder how this would affect
Windows folks.

What are the paths valued configuration variables other than excludesfile
that we would want to support?  There was a topic to allow mail-aliases
lookup for parameters given to the "--author" option today, and send-email
takes aliasfile configuration.  Because the latter is a script, we would
need a "--path" option to "git config" (the idea is similar to existing
"--bool" option) so that calling scripts can ask the same "magic"
performed to configuration variables' values before being reported.

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

[Karl Chen]

>>>>> On 2008-08-22 14:10 PDT, Junio C Hamano writes:

    Junio> Karl Chen <quarl@cs.berkeley.edu> writes:
    >> Another idea is to have a non-absolute path be interpreted
    >> relative to the location of .gitconfig.

    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?

    Junio> I am not sure what a relative path in "/etc/gitconfig"
    Junio> should be relative to, though.

Why not just relative to the location of that file?  Normally
/etc, but if some distro customizes the location of /etc/gitconfig
(/etc/git/config), or on non-Linux/posix systems it's somewhere
else, or git is installed in /usr/local or /opt or $HOME, then
it's still relative to the location of system gitconfig.

    Junio> However, this has a technical difficulty.  When
    Junio> configuration values are read, the code that knows what
    Junio> the value means does not in general know which
    Junio> configuration file is being read from.

Sounds like a refactoring issue.

    Junio> It is quite likely that somebody would want you to
    Junio> interpret "~name/" if you advertize that you support
    Junio> "~/", so you would need to call getpwuid() eventually
    Junio> if you go down this path.  I wonder how this would
    Junio> affect Windows folks.

I would be happy either way.  Though since git uses getenv("HOME")
to find ~/.gitconfig, I can see arguments for looking for the
ignore file there also, in case it's different.

    Junio> we would need a "--path" option to "git config" (the
    Junio> idea is similar to existing "--bool" option) so that
    Junio> calling scripts can ask the same "magic" performed to
    Junio> configuration variables' values before being reported.

Sounds fine.

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

Relative paths and ~ (and $HOME) are all mutually compatible so
they could all be implemented.  If $HOME were supported directly
(either just "$HOME" or parsing all $ENVVARS) then it'd be easier
to decide to use getpwuid for ~.  Personally I'd use: 1) relative
path, 2) $HOME (as "~" or "$HOME"), 3) getpwuid (as "~")

Karl

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.

limiting relationship of git dir and worktree (was Re: [PATCH] Support "core.excludesfile = ~/.gitignore")

[Jeff King]

On Sun, Aug 24, 2008 at 04:40:21PM -0700, Junio C Hamano wrote:

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

Well, as a non-user of this feature, I certainly have no argument
against taking it out. Maybe the subject line will pull some other
people into the discussion.

-Peff

Dropping core.worktree and GIT_WORK_TREE support (was Re: limiting relationship of git dir and worktree)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Aug 24, 2008 at 04:40:21PM -0700, Junio C Hamano wrote:
>
>> 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.
>
> Well, as a non-user of this feature, I certainly have no argument
> against taking it out. Maybe the subject line will pull some other
> people into the discussion.

Heh, if we are to do the attention-getter, let's do so more strongly ;-)

Re: Dropping core.worktree and GIT_WORK_TREE support (was Re: limiting relationship of git dir and worktree)

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 574 bytes --]

On Sun, Aug 24, 2008 at 05:30:54PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Heh, if we are to do the attention-getter, let's do so more strongly ;-)

Does this include removing of --work-tree as well?

The git backend of Pootle (http://translate.sourceforge.net/wiki/) uses
it.

Also, here is a question:

$ git --git-dir git/.git --work-tree git diff --stat|tail -n 1
 1443 files changed, 0 insertions(+), 299668 deletions(-)

So, it's like it thinks every file is removed.

But then:

$ cd git
$ git diff --stat|wc -l
0

is this a bug, or a user error?

Thanks.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: Dropping core.worktree and GIT_WORK_TREE support

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> On Sun, Aug 24, 2008 at 05:30:54PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
>> Heh, if we are to do the attention-getter, let's do so more strongly ;-)
>
> Does this include removing of --work-tree as well?
>
> The git backend of Pootle (http://translate.sourceforge.net/wiki/) uses
> it.

Interesting.  Does it use it because it can (meaning, --work-tree is
supposed to work), or because --work-tree is the cleanest way to do what
it wants to do (if the feature worked properly, that is, which is not the
case)?

> Also, here is a question:
>
> $ git --git-dir git/.git --work-tree git diff --stat|tail -n 1
>  1443 files changed, 0 insertions(+), 299668 deletions(-)
>
> So, it's like it thinks every file is removed.
>
> But then:
>
> $ cd git
> $ git diff --stat|wc -l
> 0
>
> is this a bug, or a user error?

I  think it is among the many other things that falls into "the two
attempts still haven't resolved" category.

Re: Dropping core.worktree and GIT_WORK_TREE support

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 1695 bytes --]

On Sun, Aug 24, 2008 at 08:05:03PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> > Does this include removing of --work-tree as well?
> >
> > The git backend of Pootle (http://translate.sourceforge.net/wiki/) uses
> > it.
> 
> Interesting.  Does it use it because it can (meaning, --work-tree is
> supposed to work), or because --work-tree is the cleanest way to do what
> it wants to do (if the feature worked properly, that is, which is not the
> case)?

It's like:

The current working directory is like
/usr/lib/python2.5/site-packages/Pootle. The git repository is under
/some/other/path/outside/usr.

Then Pootle has two possibilities:

1) save the current directory, change to /some/other, execute git, and
change the directory back

2) use git --work-tree / --git-dir

I guess the second form is more elegant. Of course if it is decided that
this option will be removed then the old form can be still used, but I
think that would be a step back.

> > Also, here is a question:
> >
> > $ git --git-dir git/.git --work-tree git diff --stat|tail -n 1
> >  1443 files changed, 0 insertions(+), 299668 deletions(-)
> >
> > So, it's like it thinks every file is removed.
> >
> > But then:
> >
> > $ cd git
> > $ git diff --stat|wc -l
> > 0
> >
> > is this a bug, or a user error?
> 
> I  think it is among the many other things that falls into "the two
> attempts still haven't resolved" category.

I'm unfamiliar with this part of the codebase, so in case somebody other
could look at it, that would be great, but I'm happy with write a
testcase for it. (Or in case nobody cares, I can try to fix it, but that
may take a bit more time.)

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: Dropping core.worktree and GIT_WORK_TREE support

[Nguyen Thai Ngoc Duy]

On 8/25/08, Miklos Vajna <vmiklos@frugalware.org> wrote:
> On Sun, Aug 24, 2008 at 08:05:03PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
>  > > Does this include removing of --work-tree as well?
>  > >
>  > > The git backend of Pootle (http://translate.sourceforge.net/wiki/) uses
>  > > it.
>  >
>  > Interesting.  Does it use it because it can (meaning, --work-tree is
>  > supposed to work), or because --work-tree is the cleanest way to do what
>  > it wants to do (if the feature worked properly, that is, which is not the
>  > case)?
>
>
> It's like:
>
>  The current working directory is like
>  /usr/lib/python2.5/site-packages/Pootle. The git repository is under
>  /some/other/path/outside/usr.
>
>  Then Pootle has two possibilities:
>
>  1) save the current directory, change to /some/other, execute git, and
>  change the directory back
>
>  2) use git --work-tree / --git-dir
>
>  I guess the second form is more elegant. Of course if it is decided that
>  this option will be removed then the old form can be still used, but I
>  think that would be a step back.
>
>
>  > > Also, here is a question:
>  > >
>  > > $ git --git-dir git/.git --work-tree git diff --stat|tail -n 1
>  > >  1443 files changed, 0 insertions(+), 299668 deletions(-)
>  > >
>  > > So, it's like it thinks every file is removed.
>  > >
>  > > But then:
>  > >
>  > > $ cd git
>  > > $ git diff --stat|wc -l
>  > > 0
>  > >
>  > > is this a bug, or a user error?
>  >
>  > I  think it is among the many other things that falls into "the two
>  > attempts still haven't resolved" category.
>
>
> I'm unfamiliar with this part of the codebase, so in case somebody other
>  could look at it, that would be great, but I'm happy with write a
>  testcase for it. (Or in case nobody cares, I can try to fix it, but that
>  may take a bit more time.)

Because "git diff" did not call setup_work_tree(). The same happens
for "git diff-index" that someone reported recently. IIRC "git
diff-files" has the same problem.
-- 
Duy

[PATCH] git diff/diff-index/diff-files: call setup_work_tree()

[Miklos Vajna]

This makes it possible to use git diff when we are outside the repo but
--work-tree and --git-dir is used.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Mon, Aug 25, 2008 at 08:52:11PM +0700, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> Because "git diff" did not call setup_work_tree(). The same happens
> for "git diff-index" that someone reported recently. IIRC "git
> diff-files" has the same problem.

Thanks, that was the problem.

 builtin-diff-files.c |    1 +
 builtin-diff-index.c |    1 +
 builtin-diff.c       |    1 +
 3 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/builtin-diff-files.c b/builtin-diff-files.c
index 9bf10bb..4802e00 100644
--- a/builtin-diff-files.c
+++ b/builtin-diff-files.c
@@ -19,6 +19,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 	int result;
 	unsigned options = 0;
 
+	setup_work_tree();
 	init_revisions(&rev, prefix);
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	rev.abbrev = 0;
diff --git a/builtin-diff-index.c b/builtin-diff-index.c
index 17d851b..b8e0656 100644
--- a/builtin-diff-index.c
+++ b/builtin-diff-index.c
@@ -16,6 +16,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	int i;
 	int result;
 
+	setup_work_tree();
 	init_revisions(&rev, prefix);
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	rev.abbrev = 0;
diff --git a/builtin-diff.c b/builtin-diff.c
index 7ffea97..86f9255 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -244,6 +244,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	int nongit;
 	int result = 0;
 
+	setup_work_tree();
 	/*
 	 * We could get N tree-ish in the rev.pending_objects list.
 	 * Also there could be M blobs there, and P pathspecs.
-- 
1.6.0.rc3.17.gc14c8.dirty

Re: [PATCH] git diff/diff-index/diff-files: call setup_work_tree()

[Nguyen Thai Ngoc Duy]

On 8/25/08, Miklos Vajna <vmiklos@frugalware.org> wrote:
>  diff --git a/builtin-diff-index.c b/builtin-diff-index.c
>  index 17d851b..b8e0656 100644
>  --- a/builtin-diff-index.c
>  +++ b/builtin-diff-index.c
>  @@ -16,6 +16,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
>         int i;
>         int result;
>
>  +       setup_work_tree();
>         init_revisions(&rev, prefix);
>         git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
>         rev.abbrev = 0;

I think this is only needed when cached == 0

>  diff --git a/builtin-diff.c b/builtin-diff.c
>  index 7ffea97..86f9255 100644
>  --- a/builtin-diff.c
>  +++ b/builtin-diff.c
>  @@ -244,6 +244,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>         int nongit;
>         int result = 0;
>
>  +       setup_work_tree();
>         /*
>          * We could get N tree-ish in the rev.pending_objects list.
>          * Also there could be M blobs there, and P pathspecs.
>

No. git-diff has too many modes, some does not need worktree. This
forces worktree on all modes.
-- 
Duy

Re: [PATCH] git diff/diff-index/diff-files: call setup_work_tree()

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]

On Mon, Aug 25, 2008 at 09:46:37PM +0700, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> On 8/25/08, Miklos Vajna <vmiklos@frugalware.org> wrote:
> >  diff --git a/builtin-diff-index.c b/builtin-diff-index.c
> >  index 17d851b..b8e0656 100644
> >  --- a/builtin-diff-index.c
> >  +++ b/builtin-diff-index.c
> >  @@ -16,6 +16,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
> >         int i;
> >         int result;
> >
> >  +       setup_work_tree();
> >         init_revisions(&rev, prefix);
> >         git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
> >         rev.abbrev = 0;
> 
> I think this is only needed when cached == 0
> 
> >  diff --git a/builtin-diff.c b/builtin-diff.c
> >  index 7ffea97..86f9255 100644
> >  --- a/builtin-diff.c
> >  +++ b/builtin-diff.c
> >  @@ -244,6 +244,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
> >         int nongit;
> >         int result = 0;
> >
> >  +       setup_work_tree();
> >         /*
> >          * We could get N tree-ish in the rev.pending_objects list.
> >          * Also there could be M blobs there, and P pathspecs.
> >
> 
> No. git-diff has too many modes, some does not need worktree. This
> forces worktree on all modes.

Ah, yes. I just wanted to say that I forgot do a 'make test' and
actually this breaks at least t0020-crlf.sh. I'll post a fixed patch in
a bit.

Sorry.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] git diff/diff-index/diff-files: call setup_work_tree()

[Miklos Vajna]

This makes it possible to use git diff when we are outside the repo but
--work-tree and --git-dir is used.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-diff-files.c |    1 +
 builtin-diff-index.c |    2 ++
 builtin-diff.c       |    1 +
 3 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/builtin-diff-files.c b/builtin-diff-files.c
index 9bf10bb..4802e00 100644
--- a/builtin-diff-files.c
+++ b/builtin-diff-files.c
@@ -19,6 +19,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 	int result;
 	unsigned options = 0;
 
+	setup_work_tree();
 	init_revisions(&rev, prefix);
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	rev.abbrev = 0;
diff --git a/builtin-diff-index.c b/builtin-diff-index.c
index 17d851b..5510291 100644
--- a/builtin-diff-index.c
+++ b/builtin-diff-index.c
@@ -29,6 +29,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 		else
 			usage(diff_cache_usage);
 	}
+	if (!cached)
+		setup_work_tree();
 	if (!rev.diffopt.output_format)
 		rev.diffopt.output_format = DIFF_FORMAT_RAW;
 
diff --git a/builtin-diff.c b/builtin-diff.c
index 7ffea97..57da6ed 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -279,6 +279,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	diff_no_index(&rev, argc, argv, nongit, prefix);
 
 	/* Otherwise, we are doing the usual "git" diff */
+	setup_work_tree();
 	rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
 
 	if (nongit)
-- 
1.6.0.rc3.17.gc14c8.dirty

Re: [PATCH] git diff/diff-index/diff-files: call setup_work_tree()

[Nguyen Thai Ngoc Duy]

On 8/25/08, Miklos Vajna <vmiklos@frugalware.org> wrote:
>  diff --git a/builtin-diff.c b/builtin-diff.c
>
> index 7ffea97..57da6ed 100644
>
> --- a/builtin-diff.c
>  +++ b/builtin-diff.c
>
> @@ -279,6 +279,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>         diff_no_index(&rev, argc, argv, nongit, prefix);
>
>         /* Otherwise, we are doing the usual "git" diff */
>  +       setup_work_tree();
>         rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
>
>         if (nongit)

At least builtin_diff_blobs() and builtin_diff_tree() won't need
worktree, so NACK again. Anyway I'm not familiar with diff*. Junio
should know these better.

-- 
Duy

Re: [PATCH] git diff/diff-index/diff-files: call setup_work_tree()

[Junio C Hamano]

"Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:

> On 8/25/08, Miklos Vajna <vmiklos@frugalware.org> wrote:
>>  diff --git a/builtin-diff.c b/builtin-diff.c
>>
>> index 7ffea97..57da6ed 100644
>>
>> --- a/builtin-diff.c
>>  +++ b/builtin-diff.c
>>
>> @@ -279,6 +279,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>>         diff_no_index(&rev, argc, argv, nongit, prefix);
>>
>>         /* Otherwise, we are doing the usual "git" diff */
>>  +       setup_work_tree();
>>         rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
>>
>>         if (nongit)
>
> At least builtin_diff_blobs() and builtin_diff_tree() won't need
> worktree, so NACK again. Anyway I'm not familiar with diff*. Junio
> should know these better.

How about doing it this way then?

 * diff-files is about comparing with work tree, so it obviously needs a
   work tree;

 * diff-index also does;

 * no-index is about random files outside git context, so it obviously
   doesn't need any work tree;

 * comparing two (or more) trees doesn't;

 * comparing two blobs doesn't;

 * comparing a blob with a random file doesn't;

What could be problematic is "git diff --cached".  Strictly speaking, it
compares the index and a tree so it shouldn't need any work tree.  The
same obviously applies to "git diff-index --cached".

While it is theoretically possible to have an index in a bare repository
and build your history using it without using any worktree, I do not think
it is a use case worth worrying about.  As long as setup_work_tree() does
not complain and die in such a setup, "diff --cached" itself won't look at
the work tree (whereever random place setup_work_tree() sets it) at all,
so probably it is a non issue.  I dunno.

I do not have a test environment that uses a separate worktree settings,
so this is obviously untested.

Perhaps people who are interested in keeping core.worktree alive can add
test scripts in t/ somewhere to help salvaging the feature?

---

 builtin-diff.c |    3 +++
 git.c          |    4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git i/builtin-diff.c w/builtin-diff.c
index 7ffea97..06c85da 100644
--- i/builtin-diff.c
+++ w/builtin-diff.c
@@ -114,6 +114,8 @@ static int builtin_diff_index(struct rev_info *revs,
 			      int argc, const char **argv)
 {
 	int cached = 0;
+
+	setup_work_tree();
 	while (1 < argc) {
 		const char *arg = argv[1];
 		if (!strcmp(arg, "--cached"))
@@ -207,6 +209,7 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv
 	int result;
 	unsigned int options = 0;
 
+	setup_work_tree();
 	while (1 < argc && argv[1][0] == '-') {
 		if (!strcmp(argv[1], "--base"))
 			revs->max_count = 1;
diff --git i/git.c w/git.c
index 37b1d76..a8e730d 100644
--- i/git.c
+++ w/git.c
@@ -286,8 +286,8 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "count-objects", cmd_count_objects, RUN_SETUP },
 		{ "describe", cmd_describe, RUN_SETUP },
 		{ "diff", cmd_diff },
-		{ "diff-files", cmd_diff_files, RUN_SETUP },
-		{ "diff-index", cmd_diff_index, RUN_SETUP },
+		{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
+		{ "diff-index", cmd_diff_index, RUN_SETUP | NEED_WORK_TREE },
 		{ "diff-tree", cmd_diff_tree, RUN_SETUP },
 		{ "fast-export", cmd_fast_export, RUN_SETUP },
 		{ "fetch", cmd_fetch, RUN_SETUP },

[PATCH] diff*: fix worktree setup

[Nguyễn Thái Ngọc Duy]

This fixes "git diff", "git diff-files" and "git diff-index" to work
correctly under worktree setup. Because diff* family works in many modes
and not all of them require worktree, Junio made a nice summary
(with a little modification from me):

 * diff-files is about comparing with work tree, so it obviously needs a
  work tree;

 * diff-index also does, except "diff-index --cached" or "diff --cached TREE"

 * no-index is about random files outside git context, so it obviously
  doesn't need any work tree;

 * comparing two (or more) trees doesn't;

 * comparing two blobs doesn't;

 * comparing a blob with a random file doesn't;

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin-diff-index.c |    2 +
 builtin-diff.c       |    3 ++
 git.c                |    2 +-
 t/t1501-worktree.sh  |   59 ++++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 63 insertions(+), 3 deletions(-)

diff --git a/builtin-diff-index.c b/builtin-diff-index.c
index 17d851b..0483749 100644
--- a/builtin-diff-index.c
+++ b/builtin-diff-index.c
@@ -39,6 +39,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	if (rev.pending.nr != 1 ||
 	    rev.max_count != -1 || rev.min_age != -1 || rev.max_age != -1)
 		usage(diff_cache_usage);
+	if (!cached)
+		setup_work_tree();
 	if (read_cache() < 0) {
 		perror("read_cache");
 		return -1;
diff --git a/builtin-diff.c b/builtin-diff.c
index 7ffea97..037c303 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -122,6 +122,8 @@ static int builtin_diff_index(struct rev_info *revs,
 			usage(builtin_diff_usage);
 		argv++; argc--;
 	}
+	if (!cached)
+		setup_work_tree();
 	/*
 	 * Make sure there is one revision (i.e. pending object),
 	 * and there is no revision filtering parameters.
@@ -225,6 +227,7 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv
 	    (revs->diffopt.output_format & DIFF_FORMAT_PATCH))
 		revs->combine_merges = revs->dense_combined_merges = 1;
 
+	setup_work_tree();
 	if (read_cache() < 0) {
 		perror("read_cache");
 		return -1;
diff --git a/git.c b/git.c
index 37b1d76..fdb0f71 100644
--- a/git.c
+++ b/git.c
@@ -286,7 +286,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "count-objects", cmd_count_objects, RUN_SETUP },
 		{ "describe", cmd_describe, RUN_SETUP },
 		{ "diff", cmd_diff },
-		{ "diff-files", cmd_diff_files, RUN_SETUP },
+		{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
 		{ "diff-index", cmd_diff_index, RUN_SETUP },
 		{ "diff-tree", cmd_diff_tree, RUN_SETUP },
 		{ "fast-export", cmd_fast_export, RUN_SETUP },
diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index 2ee88d8..e9e352c 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -28,6 +28,7 @@ test_rev_parse() {
 	[ $# -eq 0 ] && return
 }
 
+EMPTY_TREE=$(git write-tree)
 mkdir -p work/sub/dir || exit 1
 mv .git repo.git || exit 1
 
@@ -106,12 +107,66 @@ test_expect_success 'repo finds its work tree from work tree, too' '
 '
 
 test_expect_success '_gently() groks relative GIT_DIR & GIT_WORK_TREE' '
-	cd repo.git/work/sub/dir &&
+	(cd repo.git/work/sub/dir &&
 	GIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \
 		git diff --exit-code tracked &&
 	echo changed > tracked &&
 	! GIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \
-		git diff --exit-code tracked
+		git diff --exit-code tracked)
+'
+cat > diff-index-cached.expected <<\EOF
+:000000 100644 0000000000000000000000000000000000000000 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 A	sub/dir/tracked
+EOF
+cat > diff-index.expected <<\EOF
+:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A	sub/dir/tracked
+EOF
+
+
+test_expect_success 'git diff-index' '
+	GIT_DIR=repo.git GIT_WORK_TREE=repo.git/work git diff-index $EMPTY_TREE > result &&
+	cmp diff-index.expected result &&
+	GIT_DIR=repo.git git diff-index --cached $EMPTY_TREE > result &&
+	cmp diff-index-cached.expected result
+'
+cat >diff-files.expected <<\EOF
+:100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0000000000000000000000000000000000000000 M	sub/dir/tracked
+EOF
+
+test_expect_success 'git diff-files' '
+	GIT_DIR=repo.git GIT_WORK_TREE=repo.git/work git diff-files > result &&
+	cmp diff-files.expected result
+'
+
+cat >diff-TREE.expected <<\EOF
+diff --git a/sub/dir/tracked b/sub/dir/tracked
+new file mode 100644
+index 0000000..5ea2ed4
+--- /dev/null
++++ b/sub/dir/tracked
+@@ -0,0 +1 @@
++changed
+EOF
+cat >diff-TREE-cached.expected <<\EOF
+diff --git a/sub/dir/tracked b/sub/dir/tracked
+new file mode 100644
+index 0000000..e69de29
+EOF
+cat >diff-FILES.expected <<\EOF
+diff --git a/sub/dir/tracked b/sub/dir/tracked
+index e69de29..5ea2ed4 100644
+--- a/sub/dir/tracked
++++ b/sub/dir/tracked
+@@ -0,0 +1 @@
++changed
+EOF
+
+test_expect_success 'git diff' '
+	GIT_DIR=repo.git GIT_WORK_TREE=repo.git/work git diff $EMPTY_TREE > result &&
+	cmp diff-TREE.expected result &&
+	GIT_DIR=repo.git git diff --cached $EMPTY_TREE > result &&
+	cmp diff-TREE-cached.expected result &&
+	GIT_DIR=repo.git GIT_WORK_TREE=repo.git/work git diff > result &&
+	cmp diff-FILES.expected result
 '
 
 test_done
-- 
1.6.0.96.g2fad1.dirty

Re: Dropping core.worktree and GIT_WORK_TREE support

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> On Sun, Aug 24, 2008 at 08:05:03PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
>> > Does this include removing of --work-tree as well?
>> >
>> > The git backend of Pootle (http://translate.sourceforge.net/wiki/) uses
>> > it.
>> 
>> Interesting.  Does it use it because it can (meaning, --work-tree is
>> supposed to work), or because --work-tree is the cleanest way to do what
>> it wants to do (if the feature worked properly, that is, which is not the
>> case)?
>
> It's like:
>
> The current working directory is like
> /usr/lib/python2.5/site-packages/Pootle. The git repository is under
> /some/other/path/outside/usr.
>
> Then Pootle has two possibilities:

The real question was about if/why that git repository _has to be_ outside
of /usr/lib/*/Pootle/.  Is that because --work-tree, if worked properly,
would have allowed it to be, or is that because for some external reason
you are not allowed to have .git under /usr/lib/*/Pootle/ directory?

If the latter, that shows the real requirement to keep supporting it as a
feature, and issues around it need to be fixed.  Otherwise, i.e. if it
does not require use of --work-tree but it uses it only because it could,
that gives us less incentive to keep --work-tree as a feature.

I haven't read the breakage and fix around diff in this thread yet, but I
will when I get home.

Thanks to both Nguyen and you for trying to salvage --work-tree support.

Re: Dropping core.worktree and GIT_WORK_TREE support

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

On Mon, Aug 25, 2008 at 02:21:54PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> The real question was about if/why that git repository _has to be_ outside
> of /usr/lib/*/Pootle/.  Is that because --work-tree, if worked properly,
> would have allowed it to be, or is that because for some external reason
> you are not allowed to have .git under /usr/lib/*/Pootle/ directory?

I'm not 100% sure, but I think the reason is that Pootle runs as the
user 'pootle' which has write access to some /var/pootle or /home/pootle
dir, but has no write access to /usr/lib/*/Pootle/.

> If the latter, that shows the real requirement to keep supporting it as a
> feature, and issues around it need to be fixed.  Otherwise, i.e. if it
> does not require use of --work-tree but it uses it only because it could,
> that gives us less incentive to keep --work-tree as a feature.

I think this is the latter, though as I mentioned previously - it can be
still worked around by a "cd /other/path; git <command>; cd -" (speaking
in shell commands), so it is not a "must", it would be just ugly IMHO.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: Dropping core.worktree and GIT_WORK_TREE support (was Re: limiting relationship of git dir and worktree)

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 25.08.2008 02:30:
> Jeff King <peff@peff.net> writes:
> 

>> Well, as a non-user of this feature, I certainly have no argument
>> against taking it out. Maybe the subject line will pull some other
>> people into the discussion.
> 
> Heh, if we are to do the attention-getter, let's do so more strongly ;-)

Sorry for being late to the discussion.

I think there are many use cases or environments which differ
substantially from those of the "typical" developer; this implies that
they differ from those of the typical git contributor, which naturally
leads to a certain bias in discussions like this one.

"Typical" developers track source code in the proper sense (somewhere in
$HOME); on local file systems; mostly on machines where they have root
access, or least can get extra accounts (for gitosis) or a port for "git
daemon" etc; they collaborate with peers for whom basically the same
assumptions apply.

Now think of a user say in academics, who tracks "source code" for
scientific papers (somewhere in $HOME) but also needs to track, e.g.,
central web pages or other "sources" where he has partial write access
but can't have ".git" in place (and shouldn't change ownership &
permissions), but needs to be aware of changes and log own changes; on
NFS; no extra accounts but in need of an authenticated protocol (papers
in progress are private, public only when published); who collaborates
with peers for whom the same assumptions apply, except most certainly
for git usage...

Yes, that's me, but also many others, I would think and hope, at least
increasingly so. That second scenario is one where I have to cope with
how things are set up centrally, making the best possible use of git.

I would imagine that many corporate environments are basically similar,
if individual employees want to use git without central support.

These remarks apply to the discussion about an authenticated protocol
(some way for secure, private pull&push for users with access to $HOME
and maybe cgi-bins), but also here:

I need to keep .git away from the work tree for several projects. Using
--git-dir etc. leads to problems with some commands, especially
git{k,-gui,-citool}. I found the most robust solution to be an alias
(shell) which guesses the work tree (from core.worktree etc.) and cd's
there before doing anything. This also solves the problems with diff.

I would strongly advocate for keeping the possibility of separating
git-dir and work-tree, and possibly dropping the assumption that
everything "foo.git" is a bare repo. There are config variables for
this. The Tcl/Tk family I mentioned makes even stronger assumptions. I
promise to have a look at these when I find time (oh yeah...).

Michael

Re: Dropping core.worktree and GIT_WORK_TREE support (was Re: limiting relationship of git dir and worktree)

[Jeff King]

[resend: urgh, I somehow missed the git-list when replying, so it is
cc'd here]

On Tue, Aug 26, 2008 at 09:35:50AM +0200, Michael J Gruber wrote:

> "Typical" developers track source code in the proper sense (somewhere in
> $HOME); on local file systems; mostly on machines where they have root
> access, or least can get extra accounts (for gitosis) or a port for "git
> daemon" etc; they collaborate with peers for whom basically the same
> assumptions apply.
> 
> Now think of a user say in academics, who tracks "source code" for
> scientific papers (somewhere in $HOME) but also needs to track, e.g.,
> central web pages or other "sources" where he has partial write access
> but can't have ".git" in place (and shouldn't change ownership &

Actually, I do all of those things, and I don't use the work-tree config
variable or command line options at all. :)

I think the general advice with things like web access is "don't just
dump your git stuff into a production area; instead, build and/or
install from your git work tree into your production area". Because
things like merges _can_ leave your files in a broken state.

But I do recognize that there are some special circumstances where that
isn't possible, and you are willing to accept the tradeoff. E.g., if the
checkout is extremely large and you can't afford another copy, if you
have clueless collaborators who can't understand a build procedure.

And even though I expect those cases to be the exception, it seems a
shame for git not to support split git-dir/work-tree setups because we
really are 99% there. This code has been the source of a number of
problems.

I think what is really needed is somebody to look carefully at the git
startup sequence and figure out a sane set of rules for the order of:

  - looking at env variables
  - looking at config
  - figuring out GIT_DIR and GIT_WORK_TREE
  - chdir'ing to top level of work tree if necessary

Because we obviously have some corner cases where very confusing things
are happening.

This is on my long term todo list, but my git time is very short at
least for the next few months. I think it would be great if somebody
else wanted to take the lead on this, and I would be happy to give
pointers about some of the corner cases we have already seen.

-Peff

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

[Johannes Sixt]

Karl Chen schrieb:
> +/*
> + * Expand ~ and ~user.  Returns a newly malloced string.  (If input does not
> + * start with "~", equivalent to xstrdup.)
> + */
> +static char *expand_userdir(const char *value) {

There is user_path() in path.c that does the same thing.

Watch your style: The opening brace of functions is on the next line.

-- Hannes

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

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

[Johannes Sixt]

Karl Chen schrieb:
> +		const char *after_username = strchr_or_end(path+1, '/');

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

> +		struct passwd *pw = getpw_strspan(path+1, after_username);
> +		if (!pw) return NULL;
> +		return concatstr(buf, pw->pw_dir, after_username, sz);

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

-- Hannes

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

[Karl Chen]

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

    Jeff>   2. There is no documentation update.

Relative paths and $ENVVARS would need explanation; not sure what
needs to be said about ~user since the new behavior is what people
expect to just work.  Would it go in git-config.txt if something
were added?

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>   2. There is no documentation update.
>
> Relative paths and $ENVVARS would need explanation; not sure what
> needs to be said about ~user since the new behavior is what people
> expect to just work.  Would it go in git-config.txt if something
> were added?

Traditionally, we never has interpret ~/ or ~user/.  Users expected that
they need to spell it like:

        [core]
                excludesfile = "/home/joe/.my-ignore-pattern"

Especially since this is called "variables", it is natural, without
explanation, to expect this not to work, just like this use of a variable
won't work:

        $ EDITOR='~/bin/editor' ;# notice the single quote
        $ export EDITOR;
        $ git commit -a
        fatal: exec ~/bin/editor failed.

Your patch improves the situation and now allow:

        [core]
                excludesfile = "~/.my-ignore-pattern" 

If you do not document that it is now possible for this particular
configuration variable, the users will never know.