threads / bug / 47838
Subject: [BUG] git init doesn't respect `--template` like configuration variable init.templateDir and $GIT_TEMPLATE_DIR
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.
Nguyễn Thái Ngọc Duy[PATCH 1/2] parse-options: expand $HOME on filename options
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
Nguyễn Thái Ngọc Duy[PATCH 2/2] init-db: change --template type to OPTION_FILENAME
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
Jeff KingRe: [PATCH 2/2] init-db: change --template type to OPTION_FILENAME
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
Jeff KingRe: [PATCH 1/2] parse-options: expand $HOME on filename options
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
Junio C HamanoRe: [PATCH 1/2] parse-options: expand $HOME on filename options
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
Ævar Arnfjörð BjarmasonRe: [PATCH 1/2] parse-options: expand $HOME on filename options
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=~/pathI 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.
Junio C HamanoRe: [PATCH 1/2] parse-options: expand $HOME on filename options
Æ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.
Duy NguyenRe: [PATCH 1/2] parse-options: expand $HOME on filename options
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
Ævar Arnfjörð BjarmasonRe: [PATCH 1/2] parse-options: expand $HOME on filename options
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.
Duy NguyenRe: [PATCH 1/2] parse-options: expand $HOME on filename options
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