Re: [PATCH 1/2] parse-options: expand $HOME on filename options
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 14, 2018, 18:24 UTC
- Message-ID
- <xmqqbmgro2f1.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20180214140557.GA27850@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 9 quoted lines
>> 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.
Show 26 quoted lines
>> 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