Re: [PATCH v2 3/3] parseopt: values of pathname type can be prefixed with :(optional)
- From
D. Ben Knoble <ben.knoble+github@gmail.com>
- Date
- Nov 2, 2025, 16:20 UTC
- Message-ID
- <CALnO6CC=FFuMmBfJPzunUqDOBMBtmXm3i73y9M9LgRrhxzrs9g@mail.gmail.com>
- In-Reply-To
- <e8755a04-bd44-4ead-ba44-c603bffcc75e@gmail.com>
Hi Phillip, apologies for the long delay.
On Tue, Sep 30, 2025 at 11:26 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
Show 20 quoted lines
> > Hi Ben > > On 28/09/2025 22:29, D. Ben Knoble wrote: > > From: Junio C Hamano <gitster@pobox.com> > > > > In the previous step, we introduced an optional filename that can be > > given to a configuration variable, and nullify the fact that such a > > configuration setting even existed if the named path is missing or > > empty. > > > > Let's do the same for command line options that name a pathname. > > Sounds sensible > > > +Magic filename options > > I assume we're calling these "magic" to match to pathspec "magic" > options? I wonder if that is a good idea but I don't have a better > suggestion.
Yeah, best I could come up with.
Show 8 quoted lines
> > +~~~~~~~~~~~~~~~~~~~~~~ > > +Options that take a filename allow a prefix `:(optional)`. For example: > > + > > +---------------------------- > > +git commit -F :(optional)COMMIT_EDITMSG > > +# if COMMIT_EDITMSG does not exist, equivalent to > > This doesn't quite scan for me, maybe s/, /, it is/ ?
Will include in a follow-up series now this has been merged.
Show 6 quoted lines
> > +git commit > > +---------------------------- > > + > > +Like with configuration values, if the named file is missing Git behaves as if > > I'd drop "with" here
"Like configuration values" seems strange since the subject is "Git"—other ideas?
Show 14 quoted lines
> > +the option was not given at all. See "Values" in linkgit:git-config[1].
> > +
>
> > @@ -209,21 +208,31 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
> > case OPTION_FILENAME:
> > {
> > const char *value;
> > -
> > - FREE_AND_NULL(*(char **)opt->value);
> > -
> > - err = 0;
> > + int is_optional;
>
> This can be a bool as in the last patch.Agreed.
Show 10 quoted lines
> > if (unset) > > value = NULL; > > else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) > > - value = (const char *) opt->defval; > > - else > > - err = get_arg(p, opt, flags, &value); > > + value = (char *)opt->defval; > > I'm not sure why we're changing the cast here (or why we need one in the > first place assuming opt->defval is "void*")
It looks like opt->defval is intpr_t ? At any rate, I'm not sure why the const was dropped here either. Might be an artifact of carrying an old patch forward?
A quick pickaxe search says the const qualifier is from df217ed643 (parse-opts: add OPT_FILENAME and transition builtins, 2009-05-23), unmodified by cf8c4237eb (parse-options: free previous value of `OPTION_FILENAME`, 2024-09-26). The original patch is from https://lore.kernel.org/git/20241014204427.1712182-4-gitster@pobox.com/, I think, so may just be a typo. Will fix.
Show 17 quoted lines
> > + else {
> > + int err = get_arg(p, opt, flags, &value);
> > + if (err)
> > + return err;
> > + }
> > + if (!value)
> > + return 0;
> >
> > - if (!err)
> > - *(char **)opt->value = fix_filename(p->prefix, value);
> > - return err;
> > + is_optional = skip_prefix(value, ":(optional)", &value);
> > + if (!value)
> > + is_optional = 0;
>
> I'm struggling to see how value can be NULL here as we return early if
> it NULL before calling skip_prefix()Doesn't the "skip_prefix" above write into value? So I think if "value" is exactly the string ":(optional)", then after the call to skip_prefix it points at the null terminator.
Show 6 quoted lines
> > + value = fix_filename(p->prefix, value);
> > + if (is_optional && is_empty_or_missing_file(value)) {
> > + free((char *)value);
>
> I think we want to call is_missing_file() here. If the file is missing
> then we do nothing which matches the documentation above - Good.Agreed! Missed this when editing the patches. Will fix.