| Message ID | 733c674bd1901c931a8917045eb72f661872f462.1608516320.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | negative-refspec: fix segfault on : refspec | expand |
On Sun, Dec 20, 2020 at 9:05 PM Nipunn Koorapati via GitGitGadget <gitgitgadget@gmail.com> wrote: > test_config fails to unset the configuration variable when > using --add, as it tries to run git config --unset-all --add > > Tell test_config to invoke test_unconfig with the arg $2 when > the arg $1 is --add > > Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com> > --- > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh > @@ -381,6 +381,7 @@ test_unconfig () { > config_dir=$1 > shift > fi > + echo git ${config_dir:+-C "$config_dir"} config --unset-all "$@" Stray debugging gunk? > @@ -400,7 +401,13 @@ test_config () { > - test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} '$1'" && > + > + first_arg=$1 > + if test "$1" = --add; then > + first_arg=$2 > + fi > + > + test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} '$first_arg'" && Several comments... Style on this project is to place `then` on its own line (as seen a few lines above this change): if test "$1" = --add then ... This logic would be easier to understand if the variable was named `varname` or `cfgvar` (or something), which better conveys intention, rather than `first_arg`. It feels odd to single out `--add` when there are other similar options, such as `--replace-all`, `--fixed-value`, or even `--type` which people might try using in the future. This new option parsing is somewhat brittle. If a caller uses `test_config --add -C <dir> ...`, it won't work as expected. Perhaps that's not likely to happen, but it would be easy enough to fix by unifying and generalizing option parsing a bit. Doing so would also make it easy for the other options mentioned above to be added later if ever needed. For instance: options= while test $# != 0 do case "$1" in -C) config_dir=$2 shift ;; --add) options="$options $1" ;; *) break ;; esac shift done Finally, as this is a one-off case, it might be simpler just to drop this patch altogether and open-code the cleanup in the test itself in patch [2/3] rather than bothering with test_config() in that particular case. For example: test_when_finished "test_unconfig -C two remote.one.push" && git config -C two --add remote.one.push : && test_must_fail git -C two push one && git config -C two --add remote.one.push ^refs/heads/master && git -C two push one
Eric Sunshine <sunshine@sunshineco.com> writes: > Finally, as this is a one-off case, it might be simpler just to drop > this patch altogether and open-code the cleanup in the test itself in > patch [2/3] rather than bothering with test_config() in that > particular case. For example: > > test_when_finished "test_unconfig -C two remote.one.push" && > git config -C two --add remote.one.push : && > test_must_fail git -C two push one && > git config -C two --add remote.one.push ^refs/heads/master && > git -C two push one That would be my preference, too. Thanks for carefully and patiently reviewing.
On Mon, Dec 21, 2020 at 2:00 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > Finally, as this is a one-off case, it might be simpler just to drop > > this patch altogether and open-code the cleanup in the test itself in > > patch [2/3] rather than bothering with test_config() in that > > particular case. For example: > > > > test_when_finished "test_unconfig -C two remote.one.push" && > > git config -C two --add remote.one.push : && > > test_must_fail git -C two push one && > > git config -C two --add remote.one.push ^refs/heads/master && > > git -C two push one > > That would be my preference, too. Thanks for carefully and > patiently reviewing. I forgot to mention that it likely would be a good idea to at least mention in the commit message why test_config() is not being used for that particular case. Perhaps saying something along the lines of "one test handles config cleanup manually since test_config() is not prepared to take arbitrary options such as --add" -- or something along those lines -- would be sufficient. Alternatively, an in-code comment within the test explaining the open-coding might be more helpful to people reading the code in the future.
> I forgot to mention that it likely would be a good idea to at least > mention in the commit message why test_config() is not being used for > that particular case. Perhaps saying something along the lines of "one > test handles config cleanup manually since test_config() is not > prepared to take arbitrary options such as --add" -- or something > along those lines -- would be sufficient. Alternatively, an in-code > comment within the test explaining the open-coding might be more > helpful to people reading the code in the future. I found that since test_unconfig uses --unset-all, I can write a test as such test_config -C two remote.one.push +: && test_must_fail git -C two push one && git -C two config --add remote.one.push ^refs/heads/master && git -C two push one The unconfig of the test_config will --unset-all remote.one.push. I can use this technique and add a comment to that extent. --Nipunn
On Mon, Dec 21, 2020 at 7:00 PM Nipunn Koorapati <nipunn1313@gmail.com> wrote: > I found that since test_unconfig uses --unset-all, I can write a test as such > > test_config -C two remote.one.push +: && > test_must_fail git -C two push one && > git -C two config --add remote.one.push ^refs/heads/master && > git -C two push one > > The unconfig of the test_config will --unset-all remote.one.push. I can > use this technique and add a comment to that extent. Yes, you could do that, though it is somewhat subtle and increases cognitive load since the reader has to reason about it a bit more -- and perhaps study the internal implementation of test_config() -- to convince himself or herself that the different methods of setting configuration (test_config() vs. `git config`) used in the same test is intentional and works as intended. The example presented earlier, on the other hand, in which cleanup is explicit via `test_when_finished "test_unconfig ..."` does not suffer from such increased cognitive load since it uses `git config` consistently to set configuration rather than a mix of `git config` and test_config(). This sort of consideration is important not just for reviewers, but for people who need to understand the code down the road. For this reason, I think I favor the version in which the cleanup is explicit. (But that's just my opinion...)
> Yes, you could do that, though it is somewhat subtle and increases > cognitive load since the reader has to reason about it a bit more -- > and perhaps study the internal implementation of test_config() -- to > convince himself or herself that the different methods of setting > configuration (test_config() vs. `git config`) used in the same test > is intentional and works as intended. > > The example presented earlier, on the other hand, in which cleanup is > explicit via `test_when_finished "test_unconfig ..."` does not suffer > from such increased cognitive load since it uses `git config` > consistently to set configuration rather than a mix of `git config` > and test_config(). This sort of consideration is important not just > for reviewers, but for people who need to understand the code down the > road. For this reason, I think I favor the version in which the > cleanup is explicit. (But that's just my opinion...) Totally sympathize with wanting to reduce the cognitive load of reading the test suite. As a reader of the test, I found both implementation choices nonobvious - and requiring some diving into test_config and test_unconfig (namely the --unset-all behavior). A comment on the `git config` stating that it's there because `test_config` doesn't yet support `--add` seems equally clarifying as inserting a comment next to the test_unconfig usage. That being said, I suspect anyone in the future poking around with this will have to sourcedive through test_config and test_unconfig to make sense of it. I'll switch it over to the test_unconfig option on the next reroll if requested. --Nipunn
On Mon, Dec 21, 2020 at 9:25 PM Nipunn Koorapati <nipunn1313@gmail.com> wrote: > That being said, I suspect anyone in the future poking around with > this will have to sourcedive > through test_config and test_unconfig to make sense of it. > > I'll switch it over to the test_unconfig option on the next reroll if requested. Indeed, it's not entirely uncommon to have to dig into the test functions when writing tests. My bigger concern was someone coming along and thinking that the mixed use of test_config() and manual `git config` in the same test was a mistake, and want to fix that mistake, which would lead the person down the same rabbit hole. Explicit cleanup via test_unconfig() and consistent use of `git config` within the test, on the other hand, does not look accidental, so the reader would be less likely to want to "fix the mistake". The comment you added in the re-roll above the manual cleanup saves the reader the trouble of having to dive into the implementation of test_config(), which is a nice bonus.
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 999982fe4a9..1fdd7129d51 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -381,6 +381,7 @@ test_unconfig () { config_dir=$1 shift fi + echo git ${config_dir:+-C "$config_dir"} config --unset-all "$@" git ${config_dir:+-C "$config_dir"} config --unset-all "$@" config_status=$? case "$config_status" in @@ -400,7 +401,13 @@ test_config () { config_dir=$1 shift fi - test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} '$1'" && + + first_arg=$1 + if test "$1" = --add; then + first_arg=$2 + fi + + test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} '$first_arg'" && git ${config_dir:+-C "$config_dir"} config "$@" }