Re: [PATCH] More test cases for sanitized path names

From: Robin Rosenberg <robin.rosenberg.lists@dewire.com>
Date: Feb 1, 2008, 14:17 UTC
Message-ID: <200802011517.28895.robin.rosenberg.lists@dewire.com>
In-Reply-To: <7v63x99dt9.fsf@gitster.siamese.dyndns.org>

fredagen den 1 februari 2008 skrev Junio C Hamano:

Show 30 quoted lines

> Robin Rosenberg <robin.rosenberg.lists@dewire.com> writes:
> 
> >> > +test_expect_failure 'add a directory outside the work tree' '
> >> > +	d1="$(cd .. ; pwd)" &&
> >> > +	git add "$d1"
> >> > +	echo $?
> >> > +'
> >
> > Oops. Remove the echo $?. It still fails, i.e. git add succeeds when
> > it shouldn't. I was double checking it just before sending the patch.
> 
> Ah, you found breakages.
> 
> I could not quite tell what you meant by these tests with
> test_expect_failure, either they were misuse or "currently fails
> but they shouldn't".  Coming up with a small reproducible
> failure case is 50% of solving the problem.  That's very
> appreciated.
> 
> In any case, I think the large-ish test framework update patch I
> sent tonight should go in very early post 1.5.4 cycle, so plesae
> use the new-and-improved test_expect_failure to mark these
> reproducible failure cases.  Also, there is no need for hurry
> for you to just send test cases without fixes.  When I say I do
> not take patches early, I do mean it --- I do not even take _my_
> own patches like the one you are following up on.  I've sent
> quite a many of them, and I think some are on 'pu' or 'offcuts',
> but most are only in the list archive.  If nobody cares deeply
> enough about them to test and resend with Tested-by: , I am not
> planning to go back to pick them up.

I had those test cases on my machine from my earlier work on absolute path names so I just ran them with your code and extracted those that failed and put them into your testsuite instead. That's why I sent them so early. Read them as comments on your patches, "oh you need to cover this too". It was simply very convenient for me, and hopefull for you, to supply them in patch form.

The reasone my code for absolute path names wasn't re-submitted was because I had some test cases that didn't pass. I don't really care how the problem is solved.

Show 6 quoted lines

> > Can we move the default trash one level down for all tests? That
> > would give us one free level to play with.
> 
> I'd rather not.  Most tests do not have to step outside and it
> is very handy to debug any breakage they find by always being
> able to go there with "cd t/trash".

The change would be to "cd t/trash/repo" instead. Not much different.

Show 8 quoted lines

> The updated get_pathspec() issues a warning message and returns
> the result that omits paths outside of the work tree.  It does
> not die (and it is intentional, by the way).  The callers that
> expect to always receive the same number of paths in the return
> value as argv+i they pass to get_pathspec() should be updated to
> notice that they got less than they passed in, if they care
> about this error condition, and --error-unmatch codepath is one
> of them.  I did not touch that in the weatherbaloon patch.

Git add in the current version on an absolute path outside the repo fails, so I think the updated one should. It really doesn't make sense.

ls-files outside the repo doesn't make sense either, but then the current version exits with 0 so I can't make the same reference to existing behaviour there. It just might break someone's script.

-- robin