git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ben Peart <peartben@gmail.com>
Date
May 30, 2017, 21:21 UTC
Message-ID
<bfab707a-7f3a-ca7d-1f67-66dcb6ab2ea6@gmail.com>
In-Reply-To
<CAP8UFD0OEUogQYL5Eo1kBy_1B=dBcdEJyc4B_qAUOU=2XnrxxA@mail.gmail.com>
On 5/30/2017 9:18 AM, Christian Couder wrote:
Show 41 quoted lines
> On Thu, May 25, 2017 at 8:36 PM, Ben Peart <peartben@gmail.com> wrote:
> 
> [...]
> 
>> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
>> new file mode 100755
>> index 0000000000..395db46d55
>> --- /dev/null
>> +++ b/t/t7519-status-fsmonitor.sh
>> @@ -0,0 +1,158 @@
>> +#!/bin/sh
>> +
>> +test_description='git status with file system watcher'
>> +
>> +. ./test-lib.sh
>> +
>> +clean_repo () {
>> +       git reset --hard HEAD
>> +       git clean -fd
>> +       rm -f marker
>> +}
> 
> Maybe link all the commands in the function with "&&".
> 
>> +dirty_repo () {
>> +       : >untracked
>> +       : >dir1/untracked
>> +       : >dir2/untracked
>> +       echo 1 >modified
>> +       echo 2 >dir1/modified
>> +       echo 3 >dir2/modified
>> +       echo 4 >new
>> +       echo 5 >dir1/new
>> +       echo 6 >dir2/new
>> +       git add new
>> +       git add dir1/new
>> +       git add dir2/new
>> +}
> 
> Idem.
>

I did a quick search through the existing test scripts and the majority do not link commands together with && when they are in a sub function like this. I find not having them linked together is easier to write, maintain and is more readable.

Show 36 quoted lines
>> +# The test query-fsmonitor hook proc will output a marker file we can use to
>> +# ensure the hook was actually used to generate the correct results.
>> +
>> +test_expect_success 'setup' '
>> +       mkdir -p .git/hooks &&
>> +       write_script .git/hooks/query-fsmonitor<<-\EOF &&
>> +       if [ $1 -ne 1 ]
>> +       then
>> +               echo -e "Unsupported query-fsmonitor hook version.\n" >&2
>> +               exit 1;
>> +       fi
>> +       : >marker
>> +       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

Show 24 quoted lines
>> +       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.

Show 21 quoted lines
>> +# Status is well tested elsewhere so we'll just ensure that the results are
>> +# the same when using core.fsmonitor. First call after turning on the option
>> +# does a complete scan so need to do two calls to ensure we test the new
>> +# codepath.
>> +
>> +test_expect_success 'status with core.untrackedcache true' '
> 
> If this test is using untracked cache, it should perhaps first check
> that untracked cache can be used on the current file system.
> t7063-status-untracked-cache.sh does that with the following:
> 
> test_lazy_prereq UNTRACKED_CACHE '
>      { git update-index --test-untracked-cache; ret=$?; } &&
>      test $ret -ne 1
> '
> 
> if ! test_have_prereq UNTRACKED_CACHE; then
>      skip_all='This system does not support untracked cache'
>      test_done
> fi
>

Good point. I'll change it so that untracked cache is only turned on if it is available and that the one test that requires it is skipped if it isn't available.

Show 8 quoted lines
>> +       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.

Show 13 quoted lines
>> +       clean_repo &&
>> +       git status &&
>> +       test_path_is_missing marker &&
>> +       dirty_repo &&
>> +       git status >output &&
>> +       test_path_is_file marker &&
>> +       test_i18ncmp expect output
>> +'
>> +
>> +
> 
> Spurious new line.
> 
Fixed
Show 42 quoted lines
>> +test_expect_success 'status with core.untrackedcache false' '
>> +       git config core.fsmonitor true &&
>> +       git config core.untrackedcache false &&
>> +       git -c core.fsmonitor=false -c core.untrackedcache=false status >expect &&
> 
> Again core.untrackedcache is already set to false on the previous line.
> 
>> +       clean_repo &&
>> +       git status &&
>> +       test_path_is_missing marker &&
>> +       dirty_repo &&
>> +       git status >output &&
>> +       test_path_is_file marker &&
>> +       test_i18ncmp expect output
>> +'
>> +
>> +# Ensure commands that call refresh_index() to move the index back in time
>> +# properly invalidate the fsmonitor cache
>> +
>> +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 &&
>> +       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).

Show 14 quoted lines
>> +       git -c core.fsmonitor=false status >expect &&
>> +       test_i18ncmp expect output
>> +'
>> +
>> +# Now make sure it's actually skipping the check for modified and untracked
>> +# files unless it is told about them.  Note, after "git reset --hard HEAD" no
>> +# extensions exist other than 'TREE' so do a "git status" to get the extension
>> +# written before testing the results.
>> +
>> +test_expect_success 'status doesnt detect unreported modifications' '
> 
> Maybe:
> 
> test_expect_success "status doesn't detect unreported modifications" '
Fixed
Show 20 quoted lines
> 
>> +       git config core.fsmonitor true &&
>> +       git config core.untrackedcache true &&
>> +       write_script .git/hooks/query-fsmonitor<<-\EOF &&
>> +       :>marker
>> +       EOF
>> +       clean_repo &&
>> +       git status &&
>> +       test_path_is_missing marker &&
>> +       : >untracked &&
>> +       echo 2 >dir1/modified &&
>> +       git status >output &&
>> +       test_path_is_file marker &&
>> +       test_i18ngrep ! "Changes not staged for commit:" output &&
>> +       test_i18ngrep ! "Untracked files:" output &&
>> +       write_script .git/hooks/query-fsmonitor<<-\EOF &&
>> +       :>marker
>> +       printf "untracked%s\0"
> 
> Why is there a %s in the above?
Fixed
Show 13 quoted lines
> 
>> +       printf "dir1/modified\0"
>> +       EOF
>> +       git status >output &&
>> +       test_path_is_file marker &&
>> +       test_i18ngrep "Changes not staged for commit:" output &&
>> +       test_i18ngrep "Untracked files:" output
>> +'
>> +
>> +test_done
>> --
>> 2.13.0.windows.1.9.gc201c67b71
>>
Previous: Christian CouderNext: Junio C Hamano
Message 10 of 14 in “Fast git status via a file system watcher”
  1. 0/6 Fast git status via a file system watcherBen Peart, May 25, 2017
  2. 1/6 bswap: add 64 bit endianness helper get_be64Ben Peart, May 25, 2017
  3. 6/6 fsmonitor: add a sample query-fsmonitor hook script for WatchmanBen Peart, May 25, 2017
  4. Christian CouderMay 31, 2017
  5. 3/6 fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.Ben Peart, May 25, 2017
  6. 5/6 fsmonitor: add documentation for the fsmonitor extension.Ben Peart, May 25, 2017
  7. 2/6 dir: make lookup_untracked() available outside of dir.cBen Peart, May 25, 2017
  8. 4/6 fsmonitor: add test cases for fsmonitor extensionBen Peart, May 25, 2017
  9. Christian CouderMay 30, 2017
  10. Ben PeartMay 30, 2017
  11. Junio C HamanoMay 30, 2017
  12. Ben PeartMay 31, 2017
  13. Christian CouderMay 31, 2017
  14. Ben PeartMay 31, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.