Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

On Tue, May 30, 2017 at 11:21 PM, Ben Peart [off-list ref] wrote:

On 5/30/2017 9:18 AM, Christian Couder wrote:

quoted

On Thu, May 25, 2017 at 8:36 PM, Ben Peart [off-list ref] wrote:

quoted

+       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.

quoted

quoted

+       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.

quoted

quoted

+       git config core.fsmonitor true  &&
+       git config core.untrackedcache true &&
+       git -c core.fsmonitor=false -c core.untrackedcache=true status

quoted

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.

quoted

quoted

+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.

quoted

quoted

+       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.