Jeff King <peff@xxxxxxxx> writes:
> Yes, because ":/" is treated specially in check_filename(), and avoids
> kicking in the wildcard behavior. That is certainly preferring revs to
> pathspecs, but I think preferring one over the other is preferable to
> barfing. If the user wants carefulness, they should use "--"
> unconditionally. If they want to DWIM, we should make it as painless as
> possible, even if we sometimes guess wrong.
OK, I think that is sensible.
> But I have a feeling from what you've written that you do not agree with
> the "err and allow something possibly ambiguous" philosophy.
Not anymore ;-)
>> I actually think that no_wildcard() check added in check_filename()
>> was the original mistake. If we revert the check_filename() to a
>> simple "Is this a filename?" and move the "does this thing have a
>> wildcard" aka "can this be a pathspec even when check_filename()
>> says there is no file with that exact name?" to the code that tries
>> to allow users omit "--", i.e. the caller of check_filename(), would
>> that make the code structure and the semantics much cleaner, I
>> wonder...
>
> Yes. After writing the above, I was envisioning pushing the "err on this
> side" logic into check_filename() with a flag. The main callers are
> verify_filename() and verify_non_filename(), and they would use opposite
> flags from each other. But pulling that logic out to the caller would
> be fine, too.
>
> IOW, something like this implements the "permissive" thing I wrote above
> (i.e., be inclusive when seeing if something could plausibly be a
> filename, but exclusive when complaining that it _could_ be one):
Yup, I think that is probably a better first step.
> diff --git a/setup.c b/setup.c
> index 2c4b22c..995e924 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -139,9 +139,7 @@ int check_filename(const char *prefix, const char *arg)
> if (arg[2] == '\0') /* ":/" is root dir, always exists */
> return 1;
> name = arg + 2;
> - } else if (!no_wildcard(arg))
> - return 1;
> - else if (prefix)
> + } else if (prefix)
> name = prefix_filename(prefix, strlen(prefix), arg);
> else
> name = arg;
> @@ -202,7 +200,7 @@ void verify_filename(const char *prefix,
> {
> if (*arg == '-')
> die("bad flag '%s' used after filename", arg);
> - if (check_filename(prefix, arg))
> + if (check_filename(prefix, arg) || !no_wildcard(arg))
> return;
> die_verify_filename(prefix, arg, diagnose_misspelt_rev);
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html