Re: [PATCH v3 4/4] diff --no-index: support limiting by pathspec

[Junio C Hamano <gitster@pobox.com>]

Jacob Keller <jacob.e.keller@intel.com> writes:

> The git diff --no-index mode does not support pathspecs, and cannot
> limit the diff output in this way. Other diff programs such as GNU
> difftools have options for excluding paths based on a pattern match.

True.

> However, using git diff as a diff replacement has several advantages
> over many popular diff tools, including coloring moved lines, rename
> detections, and similar.

I said this when we introduced "--no-index", but back when "git" was
still young, it would have helped a lot wider developer populations
if we didn't do "git diff --no-index" and instead donated these
features to "many popular diff tools".  We however chose to be lazy
and selfish---those who want to use these features are better off
installing "git" even if they are not using it for version control,
only to use it as "better diff".

People are still welcome to add "coloring moved lines, rename
detections and similar" to "many popular diff tools", but given that
"git" has become fairly popular and widely used, it may not matter
all that much any more that we were lazy and selfish ;-).

> Teach git diff --no-index how to handle pathspecs to limit the
> comparisons. This will only be supported if both provided paths are
> directories.

Good.  If you are giving only two files, pathspec limiting would not
make much sense.  If you are giving a file and a directory, lazily
give only F and D to compare F with D/F, that is essentially giving
only two files F and D/F, so pathspec limiting would not make much
sense, either.  pathspec limited comparison would make sense only
when you are talking about two sets of files.

> However, if we always passed DO_MATCH_LEADING_PATHSPEC, then we will
> incorrectly match in certain cases such as matching 'a/c' against
> ':(glob)**/d'. The match logic will see that a matches the leading part
> of the **/ and accept this even tho c doesn't match. The trick seems to
> be setting both DO_MATCH_LEADING_PATHSPEC and DO_MATCH_DIRECTORY when
> checking directories, but set neither of them when checking files.

Sounds sensible.

> Some other gotchas and open questions:
>
>  1) pathspecs must all come after the first two path arguments, you
>     can't re-arrange them to come first. I'm treating them sort of like
>     the treeish arguments to git diff-tree.

Exactly.  "git diff" proper is about comparing two sets of files,
either comparing two tree-ishes "git diff master next", comparing a
tree-ish and the index "git diff --cached HEAD", comparing a
tree-ish and the working tree files "git diff HEAD", or comparing
the index and the working tree files "git diff".  It is a natural
extension that "git --no-index dirA dirB" compares contents of the
two directories.  In all of these forms, it is common that the
comparison can be pathspec limited by giving pathspec elements as
the command line arguments at the end.

>  2) The pathspecs are interpreted relative to the provided paths, and
>     thus will always need to be specified as relative paths, and will be
>     interpreted as relative to the root of the search for each path
>     separately.

Yes, that is not anything new or something we need to point out as
if it is any different from the "normal" pathspec.

>  3) negative pathspecs have to be fully qualified from the root, i.e.
>     ':(exclude)file' will only exclude 'a/file' and not 'a/b/file'
>     unless you also use '(glob)' or similar. I think this matches the
>     other pathspec support, but I an not 100% sure.

I think that is correct "git ls-files :(exclude)po" does not exclude
git-gui/po, for example.

> -`git diff [<options>] --no-index [--] <path> <path>`::
> +`git diff [<options>] --no-index [--] <path> <path> [<pathspec>...]`::

This is a bit unfortunate.  The disambiguating "--" should ideally
be between the "things to be compared" and the pathspec, as the
former corresponds to <rev> in the normal "git diff" invocation.

> +	... If both
> +	paths point to directories, additional pathspecs may be
> +	provided. These will limit the files included in the
> +	difference. All such pathspecs must be relative as they
> +	apply to both sides of the diff.

"as they" -> "and they"?

> +test_expect_success 'diff --no-index with pathspec' '
> +	test_expect_code 1 git diff --no-index a b 1 >actual &&
> +	cat >expect <<-EOF &&
> +	diff --git a/a/1 b/a/1
> +	deleted file mode 100644
> +	index d00491f..0000000
> +	--- a/a/1
> +	+++ /dev/null
> +	@@ -1 +0,0 @@
> +	-1
> +	EOF
> +	test_cmp expect actual
> +'

If you use --name-only or --name-status would the test become
simpler?

> +
> +test_expect_success 'diff --no-index with pathspec no matches' '
> +	test_expect_code 0 git diff --no-index a b missing
> +'

OK.

> +test_expect_success 'diff --no-index with negative pathspec' '
> +	test_expect_code 1 git diff --no-index a b ":!2" >actual &&
> +	cat >expect <<-EOF &&
> +	diff --git a/a/1 b/a/1
> +	deleted file mode 100644
> +	index d00491f..0000000
> +	--- a/a/1
> +	+++ /dev/null
> +	@@ -1 +0,0 @@
> +	-1
> +	EOF
> +	test_cmp expect actual
> +'

OK.

All other tests also look sensible.

Thanks.