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

45 messages Options
Embed this post
Permalink
1 2 3
Nguyen Thai Ngoc Duy

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

Reply Threaded More More options
Print post
Permalink
On 8/25/08, Miklos Vajna <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Karl Chen

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

Reply Threaded More More options
Print post
Permalink
In reply to this post by Junio C Hamano

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

Signed-off-by: Karl Chen <[hidden email]>
---
 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


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Junio C Hamano

Re: Dropping core.worktree and GIT_WORK_TREE support

Reply Threaded More More options
Print post
Permalink
In reply to this post by Miklos Vajna
Miklos Vajna <[hidden email]> writes:

> On Sun, Aug 24, 2008 at 08:05:03PM -0700, Junio C Hamano <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Miklos Vajna

Re: Dropping core.worktree and GIT_WORK_TREE support

Reply Threaded More More options
Print post
Permalink
On Mon, Aug 25, 2008 at 02:21:54PM -0700, Junio C Hamano <[hidden email]> 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.


attachment0 (204 bytes) Download Attachment
Johannes Sixt-2

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

Reply Threaded More More options
Print post
Permalink
In reply to this post by Karl Chen
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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael J Gruber

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

Reply Threaded More More options
Print post
Permalink
In reply to this post by Junio C Hamano
Junio C Hamano venit, vidit, dixit 25.08.2008 02:30:
> Jeff King <[hidden email]> 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Junio C Hamano

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

Reply Threaded More More options
Print post
Permalink
In reply to this post by Nguyen Thai Ngoc Duy
"Nguyen Thai Ngoc Duy" <[hidden email]> writes:

> On 8/25/08, Miklos Vajna <[hidden email]> 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 },

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff King

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

Reply Threaded More More options
Print post
Permalink
In reply to this post by Karl Chen
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff King

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

Reply Threaded More More options
Print post
Permalink
In reply to this post by Michael J Gruber
[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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Karl Chen

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

Reply Threaded More More options
Print post
Permalink
In reply to this post by Jeff King
>>>>> 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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Karl Chen

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

Reply Threaded More More options
Print post
Permalink
In reply to this post by Jeff King
>>>>> 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?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Junio C Hamano

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

Reply Threaded More More options
Print post
Permalink
Karl Chen <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Junio C Hamano

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

Reply Threaded More More options
Print post
Permalink
In reply to this post by Karl Chen
Karl Chen <[hidden email]> 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).


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Karl Chen

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

Reply Threaded More More options
Print post
Permalink

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 <[hidden email]>
---

>>>>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nguyen Thai Ngoc Duy

[PATCH] diff*: fix worktree setup

Reply Threaded More More options
Print post
Permalink
In reply to this post by Junio C Hamano
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 <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff King

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

Reply Threaded More More options
Print post
Permalink
In reply to this post by Karl Chen
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Junio C Hamano

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

Reply Threaded More More options
Print post
Permalink
Jeff King <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Sixt-2

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

Reply Threaded More More options
Print post
Permalink
In reply to this post by Karl Chen
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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Karl Chen

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

Reply Threaded More More options
Print post
Permalink
In reply to this post by Junio C Hamano

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 <[hidden email]>
---

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Junio C Hamano

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

Reply Threaded More More options
Print post
Permalink
Karl Chen <[hidden email]> 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 <[hidden email]>

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]);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
1 2 3