Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
> ---
> .. so that "git log :/" works, not so sure this is correct though
At least the first half should be in the commit log message, and also it
should contrast it against "git log -- :/".
I doubt the approach taken by this particular patch (I do not know about
the rest of the series) is correct, as it breaks the symmetry between
verify_filename() and verify_non_filename().
When given a list of command line arguments, we do:
(1) If there is "--", then no verification is needed. Everything before
the double-dash that is not a "-flag" will be interpreted as revs
(and get_sha1() will error out otherwise), and everything after the
double-dash will be used as an pathspec element.
(2) If there is no "--", then earlier ones must be all revs and cannot be
pathnames in the working tree. The first argument that is not a rev
and everything after that must be a pathname in the working tree, and
must not be interpretable as revs.
That "must be a pathname in the working tree" is what verify_filename()
does. and you say that ":/foo" is OK to be a pathname in this patch.
But shouldn't the opposite "cannot be pathnames in the working tree" check
done by verify_non_filename() also be told the same logic? If ":/foo" is
OK to be a pathname, shouldn't check_filename() call in that function also
be avoided the same way?
And once that happens, I think you will be back to square one.
"git log :/foo" is ambiguous no matter how you slice it, if you are going
to look at only the first character in the string. It could be asking to
show only commits that touch the path in the top-level directory "foo" and
its subdirectories, or it could be asking to start traversal at a commit
with "foo" in the log message.
Longhand magic pathspecs e.g. ":(icase)foo" might have better chance, but
not by a wide margin. It can be a rev that names the blob object in the
index registered for the literal path "'(icase)foo", or it could be an
element in the pathspec to match [Ff][Oo][[Oo].
> setup.c | 16 +++++++---------
> 1 files changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 27c1d47..08f605b 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -58,15 +58,8 @@ static void NORETURN die_verify_filename(const char *prefix, const char *arg)
> unsigned char sha1[20];
> unsigned mode;
>
> - /*
> - * Saying "'(icase)foo' does not exist in the index" when the
> - * user gave us ":(icase)foo" is just stupid. A magic pathspec
> - * begins with a colon and is followed by a non-alnum; do not
> - * let get_sha1_with_mode_1(only_to_die=1) to even trigger.
> - */
> - if (!(arg[0] == ':' && !isalnum(arg[1])))
> - /* try a detailed diagnostic ... */
> - get_sha1_with_mode_1(arg, sha1, &mode, 1, prefix);
> + /* try a detailed diagnostic ... */
> + get_sha1_with_mode_1(arg, sha1, &mode, 1, prefix);
>
> /* ... or fall back the most general message. */
> die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
> @@ -85,6 +78,11 @@ void verify_filename(const char *prefix, const char *arg)
> {
> if (*arg == '-')
> die("bad flag '%s' used after filename", arg);
> +
> + /* If it's magic pathspec, just assume it's file name */
> + if (arg[0] == ':' && is_pathspec_magic(arg[1]))
> + return;
> +
> if (check_filename(prefix, arg))
> return;
> die_verify_filename(prefix, arg);
--
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