| Message ID | 20180914143708.63024-3-benpeart@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup pass on special test setups | expand |
Ben Peart <benpeart@microsoft.com> writes: > +if test -n "$GIT_FSMONITOR_TEST" > +then > + if test -n "$GIT_TEST_FSMONITOR" > + then > + echo "warning: the GIT_FSMONITOR_TEST variable has been renamed to GIT_TEST_FSMONITOR" > + else > + echo "error: the GIT_FSMONITOR_TEST variable has been renamed to GIT_TEST_FSMONITOR" > + exit 1 > + fi > +fi I would have expected that, because we are now doing multiple pairs of variables in a single series, we would add a helper function that can be called like so: check_var_migration GIT_FSMONITOR_TEST GIT_TEST_FSMONITOR in the earliest step. Perhaps something like this. check_var_migration () { old_name=$1 new_name=$2 eval "old_isset=\${${old_name}:+isset}" eval "new_isset=\${${new_name}:+isset}" case "$old_isset,$new_isset" in isset,) echo >&2 "error: $old_name is now $new_name" exit 1 ;; isset,isset) # enable this, once $old_name no longer is valid anywhere # echo >&2 "warning: $old_name is now $new_name" # echo >&2 "hint: remove $old_name" ;; esac }
Junio C Hamano <gitster@pobox.com> writes: > Ben Peart <benpeart@microsoft.com> writes: > >> +if test -n "$GIT_FSMONITOR_TEST" >> +then >> + if test -n "$GIT_TEST_FSMONITOR" >> + then >> + echo "warning: the GIT_FSMONITOR_TEST variable has been renamed to GIT_TEST_FSMONITOR" >> + else >> + echo "error: the GIT_FSMONITOR_TEST variable has been renamed to GIT_TEST_FSMONITOR" >> + exit 1 >> + fi >> +fi > > I would have expected that, because we are now doing multiple pairs > of variables in a single series, we would add a helper function that > can be called like so: > > check_var_migration GIT_FSMONITOR_TEST GIT_TEST_FSMONITOR > > in the earliest step. Perhaps something like this. > > check_var_migration () { > old_name=$1 new_name=$2 > eval "old_isset=\${${old_name}:+isset}" > eval "new_isset=\${${new_name}:+isset}" > case "$old_isset,$new_isset" in > isset,) > echo >&2 "error: $old_name is now $new_name" > exit 1 ;; > isset,isset) > # enable this, once $old_name no longer is valid anywhere > # echo >&2 "warning: $old_name is now $new_name" > # echo >&2 "hint: remove $old_name" > ;; > esac > } Alternatively, we could do this, to warn and then migrate the value given to the old variable automatically to the new variable and let the test proceed. check_var_migration () { old_name=$1 new_name=$2 eval "old_isset=\${${old_name}:+isset}" eval "new_isset=\${${new_name}:+isset}" case "$old_isset,$new_isset" in isset,) echo >&2 "warning: $old_name is now $new_name" echo >&2 "hint: set $new_name too during the transition period" eval "$new_name=\$$old_name" ;; isset,isset) # do this later # echo >&2 "warning: $old_name is now $new_name" # echo >&2 "hint: remove $old_name" ;; esac }
Ben Peart <benpeart@microsoft.com> writes: > diff --git a/config.c b/config.c > index 3461993f0a..3555c63f28 100644 > --- a/config.c > +++ b/config.c > @@ -2278,7 +2278,7 @@ int git_config_get_max_percent_split_change(void) > int git_config_get_fsmonitor(void) > { > if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor)) > - core_fsmonitor = getenv("GIT_FSMONITOR_TEST"); > + core_fsmonitor = getenv("GIT_TEST_FSMONITOR"); Sorry for not noticing earlier, but unlike 4/4 that changed getenv(VAR) to git_env_bool(VAR, 0) "while at it", this leaves it to getenv(VAR), meaning "if it is set to any non-empty string, it is true". Is there a reason for this discrepancy? I _think_ the renaming should be done without getting mixed with other changes like the git_env_bool() done in 4/4. The idea to use git_env_bool() in stead of getenv() may be a good one, but then we should consistently do so when appropriate, and that would make a fine theme for another topic.
On 9/14/2018 1:15 PM, Junio C Hamano wrote: > Ben Peart <benpeart@microsoft.com> writes: > >> diff --git a/config.c b/config.c >> index 3461993f0a..3555c63f28 100644 >> --- a/config.c >> +++ b/config.c >> @@ -2278,7 +2278,7 @@ int git_config_get_max_percent_split_change(void) >> int git_config_get_fsmonitor(void) >> { >> if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor)) >> - core_fsmonitor = getenv("GIT_FSMONITOR_TEST"); >> + core_fsmonitor = getenv("GIT_TEST_FSMONITOR"); > > Sorry for not noticing earlier, but unlike 4/4 that changed > getenv(VAR) to git_env_bool(VAR, 0) "while at it", this leaves it to > getenv(VAR), meaning "if it is set to any non-empty string, it is > true". Is there a reason for this discrepancy? > The difference here is that core.fsmonitor isn't a boolean value. It is a string to a command that is executed so it can't be moved over to get_env_bool(). > I _think_ the renaming should be done without getting mixed with > other changes like the git_env_bool() done in 4/4. The idea to use > git_env_bool() in stead of getenv() may be a good one, but then we > should consistently do so when appropriate, and that would make a > fine theme for another topic. >
Ben Peart <peartben@gmail.com> writes: > The difference here is that core.fsmonitor isn't a boolean value. It > is a string to a command that is executed so it can't be moved over to > get_env_bool(). Ah, of course ;-) Then please take the following as a review comment for 4/4; checking if each getenv(VAR) should or should not become git_env_bool() and updating them should be done as a separate change for variables whether they are being renamed or not in this series. >> I _think_ the renaming should be done without getting mixed with >> other changes like the git_env_bool() done in 4/4. The idea to use >> git_env_bool() in stead of getenv() may be a good one, but then we >> should consistently do so when appropriate, and that would make a >> fine theme for another topic. >>
diff --git a/config.c b/config.c index 3461993f0a..3555c63f28 100644 --- a/config.c +++ b/config.c @@ -2278,7 +2278,7 @@ int git_config_get_max_percent_split_change(void) int git_config_get_fsmonitor(void) { if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor)) - core_fsmonitor = getenv("GIT_FSMONITOR_TEST"); + core_fsmonitor = getenv("GIT_TEST_FSMONITOR"); if (core_fsmonitor && !*core_fsmonitor) core_fsmonitor = NULL; diff --git a/t/README b/t/README index 56a417439c..47165f7eab 100644 --- a/t/README +++ b/t/README @@ -319,6 +319,10 @@ GIT_TEST_OE_DELTA_SIZE=<n> exercises the uncommon pack-objects code path where deltas larger than this limit require extra memory allocation for bookkeeping. +GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all exercises the fsmonitor +code path for utilizing a file system monitor to speed up detecting +new or changed files. + Naming Tests ------------ diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh index b3b4d83eaf..f6a856f24c 100755 --- a/t/t1700-split-index.sh +++ b/t/t1700-split-index.sh @@ -6,7 +6,7 @@ test_description='split index mode tests' # We need total control of index splitting here sane_unset GIT_TEST_SPLIT_INDEX -sane_unset GIT_FSMONITOR_TEST +sane_unset GIT_TEST_FSMONITOR test_expect_success 'enable split index' ' git config splitIndex.maxPercentChange 100 && diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh index 756beb0d8e..d77012ea6d 100755 --- a/t/t7519-status-fsmonitor.sh +++ b/t/t7519-status-fsmonitor.sh @@ -8,7 +8,7 @@ test_description='git status with file system watcher' # To run the entire git test suite using fsmonitor: # # copy t/t7519/fsmonitor-all to a location in your path and then set -# GIT_FSMONITOR_TEST=fsmonitor-all and run your tests. +# GIT_TEST_FSMONITOR=fsmonitor-all and run your tests. # # Note, after "git reset --hard HEAD" no extensions exist other than 'TREE' diff --git a/t/test-lib.sh b/t/test-lib.sh index 44288cbb59..0ef111d808 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -140,6 +140,17 @@ then export GIT_INDEX_VERSION fi +if test -n "$GIT_FSMONITOR_TEST" +then + if test -n "$GIT_TEST_FSMONITOR" + then + echo "warning: the GIT_FSMONITOR_TEST variable has been renamed to GIT_TEST_FSMONITOR" + else + echo "error: the GIT_FSMONITOR_TEST variable has been renamed to GIT_TEST_FSMONITOR" + exit 1 + fi +fi + # Add libc MALLOC and MALLOC_PERTURB test # only if we are not executing the test with valgrind if expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null ||
Rename GIT_FSMONITOR_TEST to GIT_TEST_FSMONITOR for consistency with the other GIT_TEST_ special setups and properly document its use. Add logic in t/test-lib.sh to give an error when the old variable is set to let people know they need to update their environment to use the new variable. If the new variable is also set, just give a warning so they can eventually remove the old variable. Signed-off-by: Ben Peart <Ben.Peart@microsoft.com> --- config.c | 2 +- t/README | 4 ++++ t/t1700-split-index.sh | 2 +- t/t7519-status-fsmonitor.sh | 2 +- t/test-lib.sh | 11 +++++++++++ 5 files changed, 18 insertions(+), 3 deletions(-)