Re: Fwd: Possibly nicer pathspec syntax?

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 8, 2017, 04:42 UTC
Message-ID: <xmqq4m05xph4.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CA+55aFyAEaMKA+2oPJct4ffJ0-_z2vrYxmQ9yrkbxzB3Hk6WfQ@mail.gmail.com>

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 7 quoted lines

> People don't expect set theory from their pathspecs. They expect their
> pathspecs to limit the output. They've learnt that within a
> subdirectory, the pathspec limits to that subdirectory. And now it
> suddenly starts showing things outside the subdirectory?
>
> At that point no amount of "but but think about set theory" will
> matter, methinks.

I do not feel too strongly about it, either, but when we invoke "what would people who go down into subdirectories expect", the issue becomes a lot bigger.

"git diff/log" without any pathspec in a subdirectory still shows the whole diff. "git grep" looks for hits inside that subdirectory, on the other hand.

I think the former design decision is mostly a historical accident. "The project tree as the whole is what matters" was one reason, and another is that historically we didn't have ":/"--defaulting to the whole tree and telling people to give "." was easier than defaulting to the current and telling people to give "../.." after counting how many levels deep you started at. If we were designing the system today with "." and ":/", I wouldn't be surprised if we chose "always limit to cwd if there is no pathspec" for any command for the sake of consistency. We can easily say "if you want whole-tree, pass :/" instead.

But we do not live in that world, and I do not think change them to make things consistent is what we are discussing in this thread. Given users accept and expect that "diff" without pathspec is whole tree, while "grep" without pathspec is limited to cwd, what is the reasonable definition of "everything from which negative ones are excluded"? That is the question I am trying to answer.

As Mike mentioned earlier in the thread, if we used "." (limit to cwd) for "diff/log" when there are only negative pathspec elements, the resulting UI would become inconsistent within a single command, as in my world view, giving no pathspec means "work on everything you would ordinarily work on", a positive pathspec means "among everything you would ordinarily work on, only work on the ones that match this pattern", and giving only a negative one ought to mean "among everything you would ordinarily work on, only work on the ones that do NOT match this pattern."

Show 12 quoted lines

>  pathspec.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/pathspec.c b/pathspec.c
> index 7ababb315..2a91247bc 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -224,6 +224,12 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
>  		char ch = *pos;
>  		int i;
>  
> +		// Special case alias for '!'

/* style? */

Show 16 quoted lines

> +		if (ch == '^') {
> +			*magic |= PATHSPEC_EXCLUDE;
> +			continue;
> +		}
> +
>  		if (!is_pathspec_magic(ch))
>  			break;
>  
> +	/*
> +	 * If everything is an exclude pattern, add one positive pattern
> +	 * that matches everyting. We allocated an extra one for this.
> +	 */
> +	if (nr_exclude == n) {
> +		init_pathspec_item(item + n, 0, "", 0, "");
> +		pathspec->nr++;
> +	}

I somehow do not think this is correct.  I expect

	cd t && git grep -e foo -- :^perf/

to look into things in 't' except for things in 't/perf', but the above will grab hits from ../Documentation/ etc. We need to pay attention to PATHSPEC_PREFER_CWD bit in the flags word, I think.

A real change probably wants to become a two-patch series (one for the new :^ alias, the other is for "everything except...") with log message ;-)