Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [jgit-dev] IgnoreNode.isIgnored() (Was: [Announce] JGit / EGit Release 4.11.0.201803080745-r)

On 12.03.2018 10:12, Thomas Wolf wrote:

On Mar 10, 2018, at 18:18, Matthias Sohn <matthias.sohn@xxxxxxxxx> wrote:

On Sat, Mar 10, 2018 at 4:04 PM, Rüdiger Herrmann <ruediger.herrmann@xxxxxx> wrote:
Dear JGit devs,

the following test used to work with JGit 4.10 and earlier. After updating to version 4.11, the test fails:

   @Test
   public void testIsIgnored() throws IOException {
     IgnoreNode ignoreNode = new IgnoreNode();
     ignoreNode.parse( new ByteArrayInputStream( "bin/".getBytes( StandardCharsets.UTF_8 ) ) );

     MatchResult matchResult = ignoreNode.isIgnored( "bin/Foo.class", false );

     assertEquals( MatchResult.IGNORED, matchResult );
   }

Is this a regression or am I doing something wrong?

bisecting between 4.10 and 4.11 yields the following commit causing this test to fail:

https://git.eclipse.org/r/#/c/117186/

commit 78420b7d0a65d591d00f32675efb0a42cda6c84a
Author: Marc Strapetz <marc.strapetz@xxxxxxxxxxx>
Date:   Fri Feb 23 13:34:23 2018 +0100

     Fix processing of gitignore negations

     Processing of negated rules, like !bin/ was not working correctly: they
     were interpreted too broad, resulting in unexpected untracked files
     which should actually be ignored

     Bug: 409664
     Change-Id: I0a422fd6607941461bf2175c9105a0311612efa0
     Signed-off-by: Marc Strapetz <marc.strapetz@xxxxxxxxxxx>


IMO the test is flawed. The git ignore pattern “bin/“ definitely doesn’t match the file “bin/Foo.class”.

Exclusion processing in git (and in JGit) is a recursive directory traversal: git and JGit will thus first
visit the directory “bin”, which _will_ match, and then will prune that whole subtree.

Correct exclusion information is thus available only during a TreeWalk (same for gitattributes). Simply
creating an IgnoreNode and then throwing an arbitrary path at it may give different results, as in this
case. Note that with a file pattern like “bin/*” or “bin/**” the test might work. But certainly not with a
directory pattern.

Yes. Before release 4.11, FastIgnoreRule's 'non-pathMatch' mode was used which was causing troubles: while it may have been convenient to have "bin/" ignoring the entire directory tree already in low-level code, this was exactly the problem for exclude-patterns and hence I was not able to maintain this behavior. Also, I couldn't find a counterpart in Git code.

Now, all non-test code except of DescribeCommand is using only pathMatch=true. For DescribeCommand, this behavior doesn't seem to be relevant, though, at least it should be easy to change/rewrite. Hence, for release 5, it makes sense to simplify the API, including PathMatcher itself and get rid of pathMatch-parameter completely.

Maybe that whole IgnoreNode.isIgnored() method should be removed — it isn’t called in JGit or EGit.

This could be part of the cleanup.

-Marc


Back to the top