[BUG] git init doesn't respect `--template` like configuration variable init.templateDir and $GIT_TEMPLATE_DIR

[BUG] git init doesn't respect `--template` like configuration variable init.templateDir and $GIT_TEMPLATE_DIR

[Doron Behar]

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

The title says it all.

If I run `git init --template=~/path/to/my/template` I get the following
message:

	warning: templates not found ~/path/to/my/template

But, if I run:

	$ env GIT_TEMPLATE_DIR=~/path/to/my/template git init

I don't get a warning and the template is used just fine.

If I configure the configuration variable `init.templateDir` to
`~/path/to/my/template` and run `git init` I don't get a warning and the
template is used just fine.



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 691 bytes --]

[PATCH 1/2] parse-options: expand $HOME on filename options

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

When you specify "--path ~/foo", the shell will automatically expand
~/foo to $HOME/foo before it's passed to git. The expansion is not done
on "--path=~/foo". An experienced user sees the difference but it could
still be confusing for others (especially when tab-completion still
works on --path=~/foo).

Support $HOME expansion for all filename options. There are about seven
of them.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 parse-options.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index d265a756b5..c33f14c74e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -38,10 +38,13 @@ static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,
 
 static void fix_filename(const char *prefix, const char **file)
 {
-	if (!file || !*file || !prefix || is_absolute_path(*file)
-	    || !strcmp("-", *file))
+	if (!file || !*file || is_absolute_path(*file) ||
+	    !strcmp("-", *file))
 		return;
-	*file = prefix_filename(prefix, *file);
+	if (**file == '~')
+		*file = expand_user_path(*file, 0);
+	else if (prefix)
+		*file = prefix_filename(prefix, *file);
 }
 
 static int opt_command_mode_error(const struct option *opt,
-- 
2.16.1.435.g8f24da2e1a

[PATCH 2/2] init-db: change --template type to OPTION_FILENAME

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

OPTION_FILENAME has some magic behind the scene, like prefixing which is
useless for init-db. The $HOME expansion though does come handy and
makes --template more consistent with the rest (both env and config var
get $HOME expansion).

Noticed-by: Doron Behar <doron.behar@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/init-db.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 68ff4ad75a..d6bd9f19cb 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -473,8 +473,9 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	const char *template_dir = NULL;
 	unsigned int flags = 0;
 	const struct option init_db_options[] = {
-		OPT_STRING(0, "template", &template_dir, N_("template-directory"),
-				N_("directory from which templates will be used")),
+		{ OPTION_FILENAME, 0, "template", &template_dir,
+			N_("template-directory"),
+			N_("directory from which templates will be used")},
 		OPT_SET_INT(0, "bare", &is_bare_repository_cfg,
 				N_("create a bare repository"), 1),
 		{ OPTION_CALLBACK, 0, "shared", &init_shared_repository,
-- 
2.16.1.435.g8f24da2e1a

Re: [PATCH 2/2] init-db: change --template type to OPTION_FILENAME

[Jeff King]

On Wed, Feb 14, 2018 at 05:51:49PM +0700, Nguyễn Thái Ngọc Duy wrote:

> OPTION_FILENAME has some magic behind the scene, like prefixing which is
> useless for init-db. The $HOME expansion though does come handy and
> makes --template more consistent with the rest (both env and config var
> get $HOME expansion).

Yep, makes sense.

> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 68ff4ad75a..d6bd9f19cb 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -473,8 +473,9 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
>  	const char *template_dir = NULL;
>  	unsigned int flags = 0;
>  	const struct option init_db_options[] = {
> -		OPT_STRING(0, "template", &template_dir, N_("template-directory"),
> -				N_("directory from which templates will be used")),
> +		{ OPTION_FILENAME, 0, "template", &template_dir,
> +			N_("template-directory"),
> +			N_("directory from which templates will be used")},

It's a shame we can't use the slightly more readable OPT_FILENAME(), but
it forces the use of "file" for the argument name. I wonder if it really
ought to be OPT_PATH(), and say "path", which would work more
universally.

At any rate, I'm fine with this until somebody feels like fiddling with
the macros.

-Peff

Re: [PATCH 1/2] parse-options: expand $HOME on filename options

[Jeff King]

On Wed, Feb 14, 2018 at 05:51:48PM +0700, Nguyễn Thái Ngọc Duy wrote:

> When you specify "--path ~/foo", the shell will automatically expand
> ~/foo to $HOME/foo before it's passed to git. The expansion is not done
> on "--path=~/foo". An experienced user sees the difference but it could
> still be confusing for others (especially when tab-completion still
> works on --path=~/foo).
> 
> Support $HOME expansion for all filename options. There are about seven
> of them.

I think this probably makes sense.

>  parse-options.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Should this be mentioned in the comment documenting OPT_FILENAME()?

> diff --git a/parse-options.c b/parse-options.c
> index d265a756b5..c33f14c74e 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -38,10 +38,13 @@ static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,
>  
>  static void fix_filename(const char *prefix, const char **file)
>  {
> -	if (!file || !*file || !prefix || is_absolute_path(*file)
> -	    || !strcmp("-", *file))
> +	if (!file || !*file || is_absolute_path(*file) ||
> +	    !strcmp("-", *file))
>  		return;
> -	*file = prefix_filename(prefix, *file);
> +	if (**file == '~')
> +		*file = expand_user_path(*file, 0);
> +	else if (prefix)
> +		*file = prefix_filename(prefix, *file);
>  }

I thought at first this needed a final "else" clause, because we don't
assign to *file if we have neither a prefix nor a user-path. But that's
what the callers expect (and we are similarly a noop if we hit the first
conditional). So this looks right.

-Peff

Re: [PATCH 1/2] parse-options: expand $HOME on filename options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Support $HOME expansion for all filename options. There are about seven
>> of them.
>
> I think this probably makes sense.
>
>>  parse-options.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> Should this be mentioned in the comment documenting OPT_FILENAME()?

Perhaps.  I think all mention of "$HOME expansion" should be
replaced with "tilde expansion", though.  I first thought we are
expanding any environment variable and $HOME is merely an example of
it when I read the title and the log message, before seeing that the
patch just adds a call to expand_user_path().

Other than that, looks good.  Thanks for a quick enhancement and a
review.

>> diff --git a/parse-options.c b/parse-options.c
>> index d265a756b5..c33f14c74e 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -38,10 +38,13 @@ static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,
>>  
>>  static void fix_filename(const char *prefix, const char **file)
>>  {
>> -	if (!file || !*file || !prefix || is_absolute_path(*file)
>> -	    || !strcmp("-", *file))
>> +	if (!file || !*file || is_absolute_path(*file) ||
>> +	    !strcmp("-", *file))
>>  		return;
>> -	*file = prefix_filename(prefix, *file);
>> +	if (**file == '~')
>> +		*file = expand_user_path(*file, 0);
>> +	else if (prefix)
>> +		*file = prefix_filename(prefix, *file);
>>  }
>
> I thought at first this needed a final "else" clause, because we don't
> assign to *file if we have neither a prefix nor a user-path. But that's
> what the callers expect (and we are similarly a noop if we hit the first
> conditional). So this looks right.
>
> -Peff

Re: [PATCH 1/2] parse-options: expand $HOME on filename options

[Ævar Arnfjörð Bjarmason]

On Wed, Feb 14 2018, Nguyễn Thái Ngọc Duy jotted:

> When you specify "--path ~/foo", the shell will automatically expand
> ~/foo to $HOME/foo before it's passed to git. The expansion is not done
> on "--path=~/foo". An experienced user sees the difference but it could
> still be confusing for others (especially when tab-completion still
> works on --path=~/foo).
>
> Support $HOME expansion for all filename options. There are about seven
> of them.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  parse-options.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index d265a756b5..c33f14c74e 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -38,10 +38,13 @@ static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,
>
>  static void fix_filename(const char *prefix, const char **file)
>  {
> -	if (!file || !*file || !prefix || is_absolute_path(*file)
> -	    || !strcmp("-", *file))
> +	if (!file || !*file || is_absolute_path(*file) ||
> +	    !strcmp("-", *file))
>  		return;
> -	*file = prefix_filename(prefix, *file);
> +	if (**file == '~')
> +		*file = expand_user_path(*file, 0);
> +	else if (prefix)
> +		*file = prefix_filename(prefix, *file);
>  }
>
>  static int opt_command_mode_error(const struct option *opt,

On current versions of git:

    (
        mkdir '/tmp/~' &&
        cd /tmp &&
        touch '~/foo' &&
        git init --template=~
    )

Will create a /tmp/.git with a 'foo' file, whereas now it'll expand ~ to
$HOME. Since this changes the behavior of this and presumably some other
options it seems like something we should have a test for.

In general I'm mildly negative on adding this, for every user like Doron
who'll be less confused by a hack like this, you'll have other users
who'll be confused about git inexplicably working with ~ in the middle
of strings, even though;

    $ echo git init --template ~/path
    git init --template /home/avar/path
    $ echo git init --template=~/path
    git init --template=~/path

I think it makes more sense to just leave such expansion to the shell,
and not try to magically expand it after the fact, since it's both
confusing (user: why does this work with git and not this other
program?), and as shown above changes existing semantics.

We'll also be setting ourselves up for more disappointed users who'll
notice that e.g. `git clone file://~/path` doesn't work, but `git clone
file://$HOME/path` does, requiring more hacks to expand ~ in more
codepaths. Will they also expact `git log -G~` to find references to
their homedir in their dotfiles.git?

I think this way lies madness, and it's better to just avoid it.

But I think that if we're going to keep it it needs some tests & docs to
point confused users to.

Re: [PATCH 1/2] parse-options: expand $HOME on filename options

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> In general I'm mildly negative on adding this, for every user like Doron
> who'll be less confused by a hack like this, you'll have other users
> who'll be confused about git inexplicably working with ~ in the middle
> of strings, even though;
>
>     $ echo git init --template ~/path
>     git init --template /home/avar/path
>     $ echo git init --template=~/path
>     git init --template=~/path
>
> I think it makes more sense to just leave such expansion to the shell,
> and not try to magically expand it after the fact, since it's both
> confusing (user: why does this work with git and not this other
> program?), and as shown above changes existing semantics.

The above certainly is a reasonable argument.

> I think this way lies madness, and it's better to just avoid it.

Re: [PATCH 1/2] parse-options: expand $HOME on filename options

[Duy Nguyen]

On Thu, Feb 15, 2018 at 5:46 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> In general I'm mildly negative on adding this, for every user like Doron
> who'll be less confused by a hack like this, you'll have other users
> who'll be confused about git inexplicably working with ~ in the middle
> of strings, even though;
>
>     $ echo git init --template ~/path
>     git init --template /home/avar/path
>     $ echo git init --template=~/path
>     git init --template=~/path

If you have a directory named '~', I expect you are already used to
prefixing it with './' because '~' will be expanded in many places
where you might want to avoid.

> I think it makes more sense to just leave such expansion to the shell,
> and not try to magically expand it after the fact, since it's both
> confusing (user: why does this work with git and not this other
> program?), and as shown above changes existing semantics.
>
> We'll also be setting ourselves up for more disappointed users who'll
> notice that e.g. `git clone file://~/path` doesn't work, but `git clone
> file://$HOME/path` does, requiring more hacks to expand ~ in more
> codepaths. Will they also expact `git log -G~` to find references to
> their homedir in their dotfiles.git?
>
> I think this way lies madness, and it's better to just avoid it.

Well. That's a bit extreme, I think if we add this then we handle case
by case in future when it makes sense, not blindly expanding '~'
everywhere.

The problem I have with this --template=~/path is tab-completion
actually completes the path, which (mis)leads me to think the command
will accept '~/' too. But this looks like a bug in git-completion.bash
though, it's a bit eager in completing stuff (or maybe it completes
"--template ~/path" and "--template=~/path" the same way).

I don't feel strongly about this. I'm OK with dropping these patches
if people think it's not a good idea (then I will try to fix
git-completion.bash not to complete '~' in this case).

> But I think that if we're going to keep it it needs some tests & docs to
> point confused users to.
-- 
Duy

Re: [PATCH 1/2] parse-options: expand $HOME on filename options

[Ævar Arnfjörð Bjarmason]

On Thu, Feb 22 2018, Duy Nguyen jotted:

> On Thu, Feb 15, 2018 at 5:46 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> In general I'm mildly negative on adding this, for every user like Doron
>> who'll be less confused by a hack like this, you'll have other users
>> who'll be confused about git inexplicably working with ~ in the middle
>> of strings, even though;
>>
>>     $ echo git init --template ~/path
>>     git init --template /home/avar/path
>>     $ echo git init --template=~/path
>>     git init --template=~/path
>
> If you have a directory named '~', I expect you are already used to
> prefixing it with './' because '~' will be expanded in many places
> where you might want to avoid.

Indeed. I've never had this use-case, just saying if it's being changed
it makes sense to have a small test for it somewhere.

>> I think it makes more sense to just leave such expansion to the shell,
>> and not try to magically expand it after the fact, since it's both
>> confusing (user: why does this work with git and not this other
>> program?), and as shown above changes existing semantics.
>>
>> We'll also be setting ourselves up for more disappointed users who'll
>> notice that e.g. `git clone file://~/path` doesn't work, but `git clone
>> file://$HOME/path` does, requiring more hacks to expand ~ in more
>> codepaths. Will they also expact `git log -G~` to find references to
>> their homedir in their dotfiles.git?
>>
>> I think this way lies madness, and it's better to just avoid it.
>
> Well. That's a bit extreme, I think if we add this then we handle case
> by case in future when it makes sense, not blindly expanding '~'
> everywhere.
>
> The problem I have with this --template=~/path is tab-completion
> actually completes the path, which (mis)leads me to think the command
> will accept '~/' too. But this looks like a bug in git-completion.bash
> though, it's a bit eager in completing stuff (or maybe it completes
> "--template ~/path" and "--template=~/path" the same way).

Ah I see, so you're doing "git init --template=~/<TAB>".

> I don't feel strongly about this. I'm OK with dropping these patches
> if people think it's not a good idea (then I will try to fix
> git-completion.bash not to complete '~' in this case).

I don't feel strongly about it either, just mildly negative on
introducing magic that gives you different behavior than shells do by
default.

I wonder if the consistency with the tab completion wouldn't be better
done by teaching the tab completion to just expand --template=~/<TAB> to
e.g. --template=/home/duy/.

On my (Debian) system doing e.g.:

    echo $HOME/bin/<TAB>

Will expand to:

    echo /home/avar/bin/

Maybe we could intercept that in the completion and ~ to the value of
$HOME. It would give completion that did the right thing, without the
expectation that ~ is going to be magic in some places and not others.

>> But I think that if we're going to keep it it needs some tests & docs to
>> point confused users to.

Re: [PATCH 1/2] parse-options: expand $HOME on filename options

[Duy Nguyen]

On Thu, Feb 22, 2018 at 8:38 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Ah I see, so you're doing "git init --template=~/<TAB>".
>
> ...
>
> I wonder if the consistency with the tab completion wouldn't be better
> done by teaching the tab completion to just expand --template=~/<TAB> to
> e.g. --template=/home/duy/.
>
> On my (Debian) system doing e.g.:
>
>     echo $HOME/bin/<TAB>
>
> Will expand to:
>
>     echo /home/avar/bin/
>
> Maybe we could intercept that in the completion and ~ to the value of
> $HOME.

Yeah that's what I had in mind (though I still have no idea if it's
hard to do). Let's drop this series then. I'll keep this <TAB> thing
in my backlog and hopefully will fix it soon. I'll have to read
git-completion.bash carefully for another series anyway. And I have a
feeling that "git --completion-helper" needs to tell
git-completion.bash "this argument takes a path" before we start to
expand stuff.
-- 
Duy