On Tue, May 30, 2017 at 11:21 PM, Ben Peart <peartben@xxxxxxxxx> wrote: > > On 5/30/2017 9:18 AM, Christian Couder wrote: >> >> On Thu, May 25, 2017 at 8:36 PM, Ben Peart <peartben@xxxxxxxxx> wrote: >> >>> + printf "untracked\0" >>> + printf "dir1/untracked\0" >>> + printf "dir2/untracked\0" >>> + printf "modified\0" >>> + printf "dir1/modified\0" >>> + printf "dir2/modified\0" >>> + printf "new\0"" >>> + printf "dir1/new\0" >>> + printf "dir2/new\0" >> >> Maybe something like the following to save a few lines and remove some >> redundancies: >> >> printf "%s\0" untracked dir1/untracked dir2/untracked \ >> modified dir1/modified dir2/modified \ >> new dir1/new dir2/new >> >> or perhaps: >> >> for f in untracked modified new >> do >> printf "%s\0" "$f" "dir1/$f" "dir2/$f" >> done > > That is a clever solution that certainly is fewer lines of code. However, I > have to read the loop and think through the logic to figure out what it is > doing vs the existing implementation where what it is doing is apparent from > just glancing at the code. I was also trying to maintain consistency with > the other status test code in t7508-status.sh Ok fair enough. >>> + EOF >>> + : >tracked && >>> + : >modified && >>> + mkdir dir1 && >>> + : >dir1/tracked && >>> + : >dir1/modified && >>> + mkdir dir2 && >>> + : >dir2/tracked && >>> + : >dir2/modified && >>> + git add . && >>> + test_tick && >>> + git commit -m initial && >>> + dirty_repo >>> +' >>> + >>> +cat >.gitignore <<\EOF >>> +.gitignore >>> +expect* >>> +output* >>> +marker* >>> +EOF >> >> This could be part of the previous setup test. > > I had followed the pattern in t7508-status.sh with this but I can move it in > if that is the preferred model. Yeah, I think it is preferred these days to have all the setup code inside tests. >>> + git config core.fsmonitor true && >>> + git config core.untrackedcache true && >>> + git -c core.fsmonitor=false -c core.untrackedcache=true status >>> >expect && >> >> I don't understand why there is " -c core.untrackedcache=true" in the >> above command as you already set core.untrackedcache to true on the >> previous line. > > Defensive programming. :) The global setting was to ensure it was set when > the test sub-functions clean and dirty were called and the command line > settings were used to make it explicit what was being tested. I can remove > them if it is causing confusion. I think it is a bit confusing indeed. >>> +test_expect_success 'refresh_index() invalidates fsmonitor cache' ' >>> + git config core.fsmonitor true && >>> + git config core.untrackedcache true && >>> + clean_repo && >>> + git status && >>> + test_path_is_missing marker && >>> + dirty_repo && >>> + write_script .git/hooks/query-fsmonitor<<-\EOF && >>> + :>marker >>> + EOF >>> + git add . && >>> + git commit -m "to reset" && >>> + git status && >>> + test_path_is_file marker && Ok so "marker" is there now. >>> + git reset HEAD~1 && >>> + git status >output && >>> + test_path_is_file marker && >> >> You already checked that "marker" exists 3 lines above, and as far as >> I can see nothing could remove this file since the previous test, as >> the hook can only create it. >> So I wonder if something is missing or if this test is redundant. > > Testing it each time ensures it is being created when it is supposed to be > (ie when the test believes it is using the query-fsmonitor hook) and that it > isn't when it isn't supposed to be (ie when the hook should not be called). I would agree with that if the "marker" file was removed after the previous "test_path_is_file marker", but I don't see any "clean_repo" or "rm marker" call that removes it.