Re: [PATCH 1/6] Recognize magic pathspec as filenames

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 12, 2011, 20:49 UTC
Message-ID: <7v4nze7x61.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1318373083-13840-2-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  .. 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].

Show 37 quoted lines

>  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);