Re: [JGIT PATCH v2 19/24] Added the class AddRuleListFactory.

[Robin Rosenberg <robin.rosenberg.lists@dewire.com>]

tisdagen den 13 maj 2008 13.24.56 skrev Florian Köberle:
> Shawn O. Pearce wrote:
> > Florian Koeberle <florianskarten@web.de> wrote:
> >> +class AddRuleListFactory {
> >> +	/*
> >> +	 * The add command of git 1.5.2.5 behaves a little bit stange: "git add
> >> +	 * a/\*z" adds the file "a/b/xyz" but "git add a/x\*" does not.
You mean a/\*/x* does not?
> >> +	 * 
> >> +	 * The first is parsed as pattern "*z" for whole directory tree "a". The
> >> +	 * second is parsed as an path.
> >> +	 * 
> >> +	 */
> > 
> > Its not strange.  C Git expands each file path to its _full_ path
> > and stores that into a buffer, then runs fnmatch() for each pattern
> > on the buffer.  If fnmatch() succeeds the path is added to the index.
> > 
> > In the case above we are running a match of "a/\*.z" against
> > "a/b/xyz" and that passes.  Or we run "a/x\*" on "a/b/xyz" and it
> > fails as the sequence of characters "a/x" does not appear in the
> > string "a/b".
> > 
> > You are running into this odd corner case because you are not
> > treating the pattern passed as something that matches against the
> > entire path.  This is one reason why TreeFilter's use the entire
> > path when they process an entry for inclusion or exclusion, and why
> > TreeWalk has each AbstractTreeIterator append the current entry name
> > onto the end of the current path buffer, so we can always examine
> > the full path from the root of the repository/working directory.
> > 
> > Trying to avoid the full path in classes like ComplexFilePattern
> > is why you are running into this corner case here, and must now do
> > extra contortions to somewhat match the behavior of C Git.
> > 
> > At this point I think most of the rules package is overcomplicated
> > and overoptimized, and yet doesn't actually quite match the behavior
> > of C Git.
> > 
> 
> Still I think that the behavior of git-add is strange:
> For example, if you want to add the following file:
> a/b/c/test.txt
> Then I can do this with "a/\*.txt" or "a/b\*.txt" but not with 
> "a/\*/c/test.txt"
> 
> I know that I handle "a/b\*.txt" wrong, and I don't know a nice way to 
> implement it in the current rules framework.
> 
> I see three options:
> 1.) Let the jgit add command work in another way then git-add does.
> 2.) Don't use the rules framework to determine if a file is selected by 
> the add command.
> 3.) Completely drop the patches
> [4.) Add some evil hacks to make it working] <- I don't like that version
> 
> Please tell me which way to go, so that I don't waste even more time on 
> patches which will never make it in.

Correctness is very important. Obviously we will slip bugs trough, but
not intentionally in a part where legacy behaviour may play an important
role. With these options available I'd select number 2.

I think your observations are interesting, especially the one you make
here about a/*/c/test.txt not matching because I don't see from the git
add manual why it shouldn't. I does not look like a recent git bug either. 

btw, I should you can use the ö in your name in mail and too. It'll make
for some interesting testing of non-ascii handling that we need to take
on (another argument for not complicating handling of names too much).
I'm sure it will get pretty hairy regardless.

-- robin