Hi Joshua, On Mon, Jul 2, 2018 at 8:58 PM, Joshua Nelson <jyn514@xxxxxxxxx> wrote: The commit message could use some clarification; it currently makes the reader think you're testing all arguments of ls-tree, when you're only testing a few new ones. Alternatively, you could squash this with patch 1. > Signed-off-by: Joshua Nelson <jyn514@xxxxxxxxx> > --- > t/t3104-ls-tree-optional-args.sh | 43 ++++++++++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > create mode 100644 t/t3104-ls-tree-optional-args.sh Please note that test files should be executable, not regular files. > > diff --git t/t3104-ls-tree-optional-args.sh t/t3104-ls-tree-optional-args.sh > new file mode 100644 > index 000000000..5917563a7 > --- /dev/null > +++ t/t3104-ls-tree-optional-args.sh > @@ -0,0 +1,43 @@ > +#!/bin/sh > + > +test_description='ls-tree test (optional args) > + > +This test tries to run git-ls-tree with various combinations of options.' This description seems like it's true for all the t310*.sh tests. How does t3104 differ from t310[0-3]*.sh? > + > +. ./test-lib.sh > + > +test_expect_success 'initial setup' ' > +echo hi > test && cp test test2 && git add test test2 && git commit -m initial' > + > +# cat appends newlines after every file What is this comment in reference to? > +test_expect_success 'succeed when given no args' 'git ls-tree' > + > +test_expect_success 'succeed when given only --' 'git ls-tree' I was a little surprised that these tests only check that the command runs to completion with a 0 exit status, and do not do any verification of the output. I wasn't necessarily expected full output verification, but having a few different commits and searching for something that'd only be shown in the relevant commit would be nice. > + > +test_expect_success 'add second commit' ' > +echo hi > test3 && git add test3 && git commit -m "commit 2"' > + > +test_expect_success 'succeed when given revision' ' > +git ls-tree HEAD~1' > + > +test_expect_success 'succeed when given revision and --' ' > +git ls-tree HEAD~1 --' > + > +test_expect_success 'succeed when given -- and file' ' > +git ls-tree -- test3' > + > +test_expect_success 'do nothing when given bad files' ' > +git ls-tree -- bad_files' ...and to reiterate the point above about verifying the output is correct, how is 'doing nothing' here distinguishable from showing all the files in the current commit if you're not checking any part of the output? > + > +test_expect_success 'succeed when given file from past revision' ' > +git ls-tree HEAD~1 test' > + > +test_expect_success 'succeed when given only file' 'git ls-tree test' > + > +test_expect_success 'raise error when given bad args' ' > +test_must_fail git ls-tree HEAD HEAD --' > + > +test_expect_success 'raise error when given bad revision' ' > +test_must_fail git ls-tree bad_revision --' > + > +test_done > -- > 2.18.GIT Lots of little things to fix up, but you're off to a great start. I'm excited to be able to have ls-tree work without me having to specify HEAD all the time. :-)
