git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [BUG] git init doesn't respect `--template` like configuration variable init.templateDir and $GIT_TEMPLATE_DIR
@ 2018-02-14 10:10 Doron Behar
  2018-02-14 10:51 ` [PATCH 1/2] parse-options: expand $HOME on filename options Nguyễn Thái Ngọc Duy
  0 siblings, 1 reply; 8+ messages in thread
From: Doron Behar @ 2018-02-14 10:10 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] parse-options: expand $HOME on filename options
  2018-02-14 10:10 [BUG] git init doesn't respect `--template` like configuration variable init.templateDir and $GIT_TEMPLATE_DIR Doron Behar
@ 2018-02-14 10:51 ` Nguyễn Thái Ngọc Duy
  2018-02-14 10:51   ` [PATCH 2/2] init-db: change --template type to OPTION_FILENAME Nguyễn Thái Ngọc Duy
                     ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2018-02-14 10:51 UTC (permalink / raw)
  To: git; +Cc: doron.behar, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] init-db: change --template type to OPTION_FILENAME
  2018-02-14 10:51 ` [PATCH 1/2] parse-options: expand $HOME on filename options Nguyễn Thái Ngọc Duy
@ 2018-02-14 10:51   ` Nguyễn Thái Ngọc Duy
  2018-02-14 14:08     ` Jeff King
  2018-02-14 14:05   ` [PATCH 1/2] parse-options: expand $HOME on filename options Jeff King
  2018-02-14 22:46   ` Ævar Arnfjörð Bjarmason
  2 siblings, 1 reply; 8+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2018-02-14 10:51 UTC (permalink / raw)
  To: git; +Cc: doron.behar, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] parse-options: expand $HOME on filename options
  2018-02-14 10:51 ` [PATCH 1/2] parse-options: expand $HOME on filename options Nguyễn Thái Ngọc Duy
  2018-02-14 10:51   ` [PATCH 2/2] init-db: change --template type to OPTION_FILENAME Nguyễn Thái Ngọc Duy
@ 2018-02-14 14:05   ` Jeff King
  2018-02-14 18:24     ` Junio C Hamano
  2018-02-14 22:46   ` Ævar Arnfjörð Bjarmason
  2 siblings, 1 reply; 8+ messages in thread
From: Jeff King @ 2018-02-14 14:05 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git, doron.behar

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] init-db: change --template type to OPTION_FILENAME
  2018-02-14 10:51   ` [PATCH 2/2] init-db: change --template type to OPTION_FILENAME Nguyễn Thái Ngọc Duy
@ 2018-02-14 14:08     ` Jeff King
  0 siblings, 0 replies; 8+ messages in thread
From: Jeff King @ 2018-02-14 14:08 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git, doron.behar

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] parse-options: expand $HOME on filename options
  2018-02-14 14:05   ` [PATCH 1/2] parse-options: expand $HOME on filename options Jeff King
@ 2018-02-14 18:24     ` Junio C Hamano
  0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2018-02-14 18:24 UTC (permalink / raw)
  To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git, doron.behar

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] parse-options: expand $HOME on filename options
  2018-02-14 10:51 ` [PATCH 1/2] parse-options: expand $HOME on filename options Nguyễn Thái Ngọc Duy
  2018-02-14 10:51   ` [PATCH 2/2] init-db: change --template type to OPTION_FILENAME Nguyễn Thái Ngọc Duy
  2018-02-14 14:05   ` [PATCH 1/2] parse-options: expand $HOME on filename options Jeff King
@ 2018-02-14 22:46   ` Ævar Arnfjörð Bjarmason
  2018-02-15 18:36     ` Junio C Hamano
  2 siblings, 1 reply; 8+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-02-14 22:46 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git, doron.behar


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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] parse-options: expand $HOME on filename options
  2018-02-14 22:46   ` Ævar Arnfjörð Bjarmason
@ 2018-02-15 18:36     ` Junio C Hamano
  0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2018-02-15 18:36 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Nguyễn Thái Ngọc Duy, git, doron.behar

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-14 10:10 [BUG] git init doesn't respect `--template` like configuration variable init.templateDir and $GIT_TEMPLATE_DIR Doron Behar
2018-02-14 10:51 ` [PATCH 1/2] parse-options: expand $HOME on filename options Nguyễn Thái Ngọc Duy
2018-02-14 10:51   ` [PATCH 2/2] init-db: change --template type to OPTION_FILENAME Nguyễn Thái Ngọc Duy
2018-02-14 14:08     ` Jeff King
2018-02-14 14:05   ` [PATCH 1/2] parse-options: expand $HOME on filename options Jeff King
2018-02-14 18:24     ` Junio C Hamano
2018-02-14 22:46   ` Ævar Arnfjörð Bjarmason
2018-02-15 18:36     ` Junio C Hamano

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox