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
 wrote:
> Ah I see, so you're doing "git init --template=~/".
>
> ...
>
> I wonder if the consistency with the tab completion wouldn't be better
> done by teaching the tab completion to just expand --template=~/ to
> e.g. --template=/home/duy/.
>
> On my (Debian) system doing e.g.:
>
> echo $HOME/bin/
>
> 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  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

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
>  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=~/".

> 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=~/ to
e.g. --template=/home/duy/.

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

echo $HOME/bin/

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 15, 2018 at 5:46 AM, Ævar Arnfjörð Bjarmason
 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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  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

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

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

[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

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