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

[Junio C Hamano <gitster@pobox.com>]

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