| Message ID | 0552123fa30243d6d8d6b378991651dd6ade7de3.1704877233.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t: mark "files"-backend specific tests | expand |
Patrick Steinhardt <ps@pks.im> writes: > [[PGP Signed Part:Undecided]] > [1. text/plain] > The t1300 test suite exercises the git-config(1) tool. To do so we > overwrite ".git/config" to contain custom contents. While this is easy > enough to do, it may create problems when using a non-default repository > format because we also overwrite the repository format version as well > as any potential extensions. With the upcoming "reftable" ref backend > the result is that we may try to access refs via the "files" backend > even though the repository has been initialized with the "reftable" > backend. > > Refactor tests which access the refdb to be more robust by using their > own separate repositories, which allows us to be more careful and not > discard required extensions. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' ' > ' > > test_expect_success 'set up custom config file' ' > - CUSTOM_CONFIG_FILE="custom.conf" && > - cat >"$CUSTOM_CONFIG_FILE" <<-\EOF > + cat >"custom.conf" <<-\EOF && > [user] > custom = true > EOF > + CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path > custom.conf)" > ' From the commit message it was not clear to me this change was needed. Do you think it's worth it to add something to the commit message explaining you now need to copy the custom.conf into each seperate repository? -- Toon
On Tue, Jan 23, 2024 at 02:41:17PM +0100, Toon Claes wrote: > > Patrick Steinhardt <ps@pks.im> writes: > > > [[PGP Signed Part:Undecided]] > > [1. text/plain] > > The t1300 test suite exercises the git-config(1) tool. To do so we > > overwrite ".git/config" to contain custom contents. While this is easy > > enough to do, it may create problems when using a non-default repository > > format because we also overwrite the repository format version as well > > as any potential extensions. With the upcoming "reftable" ref backend > > the result is that we may try to access refs via the "files" backend > > even though the repository has been initialized with the "reftable" > > backend. > > > > Refactor tests which access the refdb to be more robust by using their > > own separate repositories, which allows us to be more careful and not > > discard required extensions. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > > @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' ' > > ' > > > > test_expect_success 'set up custom config file' ' > > - CUSTOM_CONFIG_FILE="custom.conf" && > > - cat >"$CUSTOM_CONFIG_FILE" <<-\EOF > > + cat >"custom.conf" <<-\EOF && > > [user] > > custom = true > > EOF > > + CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path > > custom.conf)" > > ' > > From the commit message it was not clear to me this change was needed. > Do you think it's worth it to add something to the commit message > explaining you now need to copy the custom.conf into each seperate > repository? Good point in fact. The problem here is that before, CUSTOM_CONFIG_FILE was using a relative path that wouldn't be found when cd'ing into the respective subrepositories. By using `path-utils real_path` we resolve the relative path to the full path, and thus we can find the file regardless of our shell's current working directory. Not sure whether this is worth a reroll, but in case you or others think that it is then I'm happy to add this explanation. Patrick
On Wed, Jan 10, 2024 at 10:01 AM Patrick Steinhardt <ps@pks.im> wrote: > > The t1300 test suite exercises the git-config(1) tool. To do so we > overwrite ".git/config" to contain custom contents. Here "we" means "tests in t1300" I guess. > While this is easy > enough to do, it may create problems when using a non-default repository > format because we also overwrite the repository format version as well > as any potential extensions. But I am not sure that "we" in the above sentence is also "tests in t1300". I think overwriting the repo format version and potential extensions is done by other tests, right? Anyway it would be nice to clarify this. > With the upcoming "reftable" ref backend > the result is that we may try to access refs via the "files" backend > even though the repository has been initialized with the "reftable" > backend. Not sure here also what "we" is. When could refs be accessed via the "files" backend even though the repo was initialized with the "reftable" backend? Does this mean that some of the tests in t1300 try to access refs via the "files" backend while we may want to run all the tests using the reftable backend? > Refactor tests which access the refdb to be more robust by using their > own separate repositories, which allows us to be more careful and not > discard required extensions. Not sure what exactly is discarding extensions. Also robust is not very clear. It would be better to give at least an example of how things could fail. > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > t/t1300-config.sh | 74 ++++++++++++++++++++++++++++++----------------- > 1 file changed, 48 insertions(+), 26 deletions(-) > > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > index f4e2752134..53c3d65823 100755 > --- a/t/t1300-config.sh > +++ b/t/t1300-config.sh > @@ -1099,13 +1099,18 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' ' > ' > > test_expect_success 'check split_cmdline return' " > - git config alias.split-cmdline-fix 'echo \"' && > - test_must_fail git split-cmdline-fix && > - echo foo > foo && > - git add foo && > - git commit -m 'initial commit' && > - git config branch.main.mergeoptions 'echo \"' && > - test_must_fail git merge main > + test_when_finished 'rm -rf repo' && > + git init repo && > + ( > + cd repo && > + git config alias.split-cmdline-fix 'echo \"' && > + test_must_fail git split-cmdline-fix && > + echo foo >foo && > + git add foo && > + git commit -m 'initial commit' && > + git config branch.main.mergeoptions 'echo \"' && > + test_must_fail git merge main > + ) > " Maybe, while at it, this test could be modernized to use single quotes around the test code like: test_expect_success 'check split_cmdline return' ' ... ' or is using double quotes still Ok? > test_expect_success 'git -c "key=value" support' ' > @@ -1157,10 +1162,16 @@ test_expect_success 'git -c works with aliases of builtins' ' > ' > > test_expect_success 'aliases can be CamelCased' ' > - test_config alias.CamelCased "rev-parse HEAD" && > - git CamelCased >out && > - git rev-parse HEAD >expect && > - test_cmp expect out > + test_when_finished "rm -rf repo" && > + git init repo && > + ( > + cd repo && > + test_commit A && > + git config alias.CamelCased "rev-parse HEAD" && > + git CamelCased >out && > + git rev-parse HEAD >expect && > + test_cmp expect out > + ) > ' Here single quotes are used for example. > test_expect_success 'git -c does not split values on equals' ' > @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' ' > ' > > test_expect_success 'set up custom config file' ' > - CUSTOM_CONFIG_FILE="custom.conf" && > - cat >"$CUSTOM_CONFIG_FILE" <<-\EOF > + cat >"custom.conf" <<-\EOF && > [user] > custom = true > EOF > + CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path custom.conf)" > ' This looks like a test modernization, but maybe it's part of making the tests more robust. Anyway it might be a good idea to either talk a bit about that in the commit message or to move it to a preparatory commit if it's a modernization and other modernizations could be made in that preparatory commit. Otherwise this patch looks good to me.
On Tue, Jan 23, 2024 at 9:22 AM Patrick Steinhardt <ps@pks.im> wrote: > > On Tue, Jan 23, 2024 at 02:41:17PM +0100, Toon Claes wrote: > > > > Patrick Steinhardt <ps@pks.im> writes: > > > > > [[PGP Signed Part:Undecided]] > > > [1. text/plain] > > > The t1300 test suite exercises the git-config(1) tool. To do so we > > > overwrite ".git/config" to contain custom contents. While this is easy > > > enough to do, it may create problems when using a non-default repository > > > format because we also overwrite the repository format version as well > > > as any potential extensions. With the upcoming "reftable" ref backend > > > the result is that we may try to access refs via the "files" backend > > > even though the repository has been initialized with the "reftable" > > > backend. > > > > > > Refactor tests which access the refdb to be more robust by using their > > > own separate repositories, which allows us to be more careful and not > > > discard required extensions. > > > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > > --- > > > > > @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' ' > > > ' > > > > > > test_expect_success 'set up custom config file' ' > > > - CUSTOM_CONFIG_FILE="custom.conf" && > > > - cat >"$CUSTOM_CONFIG_FILE" <<-\EOF > > > + cat >"custom.conf" <<-\EOF && > > > [user] > > > custom = true > > > EOF > > > + CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path > > > custom.conf)" > > > ' > > > > From the commit message it was not clear to me this change was needed. > > Do you think it's worth it to add something to the commit message > > explaining you now need to copy the custom.conf into each seperate > > repository? > > Good point in fact. The problem here is that before, CUSTOM_CONFIG_FILE > was using a relative path that wouldn't be found when cd'ing into the > respective subrepositories. By using `path-utils real_path` we resolve > the relative path to the full path, and thus we can find the file > regardless of our shell's current working directory. > > Not sure whether this is worth a reroll, but in case you or others think > that it is then I'm happy to add this explanation. I also found it unclear why the CUSTOM_CONFIG_FILE change was needed. I had assumed it was a refactor to make the tests more robust. It might be nice to explain it in the commit message. :) -Justin
On Tue, Jan 23, 2024 at 05:15:48PM +0100, Christian Couder wrote: > On Wed, Jan 10, 2024 at 10:01 AM Patrick Steinhardt <ps@pks.im> wrote: > > > > The t1300 test suite exercises the git-config(1) tool. To do so we > > overwrite ".git/config" to contain custom contents. > > Here "we" means "tests in t1300" I guess. > > > While this is easy > > enough to do, it may create problems when using a non-default repository > > format because we also overwrite the repository format version as well > > as any potential extensions. > > But I am not sure that "we" in the above sentence is also "tests in > t1300". I think overwriting the repo format version and potential > extensions is done by other tests, right? Anyway it would be nice to > clarify this. > > > With the upcoming "reftable" ref backend > > the result is that we may try to access refs via the "files" backend > > even though the repository has been initialized with the "reftable" > > backend. > > Not sure here also what "we" is. When could refs be accessed via the > "files" backend even though the repo was initialized with the > "reftable" backend? Yeah, I've rephrased all of these to sey "the tests" or something similar. > Does this mean that some of the tests in t1300 try to access refs via > the "files" backend while we may want to run all the tests using the > reftable backend? Exactly. We overwrite the ".git/config", which contains the "refStorage" extension that tells us to use the "reftable" backend. So the extension is gone, and thus Git would fall back to use the "files" backend instead, which will fail. > > Refactor tests which access the refdb to be more robust by using their > > own separate repositories, which allows us to be more careful and not > > discard required extensions. > > Not sure what exactly is discarding extensions. Also robust is not > very clear. It would be better to give at least an example of how > things could fail. Hm. I don't really know how to phrase this better. The preceding paragraph already explains why we're discarding the extension and what the consequence is. I added a sentence saying ", which will cause failures when trying to access any refs." > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > t/t1300-config.sh | 74 ++++++++++++++++++++++++++++++----------------- > > 1 file changed, 48 insertions(+), 26 deletions(-) > > > > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > > index f4e2752134..53c3d65823 100755 > > --- a/t/t1300-config.sh > > +++ b/t/t1300-config.sh > > @@ -1099,13 +1099,18 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' ' > > ' > > > > test_expect_success 'check split_cmdline return' " > > - git config alias.split-cmdline-fix 'echo \"' && > > - test_must_fail git split-cmdline-fix && > > - echo foo > foo && > > - git add foo && > > - git commit -m 'initial commit' && > > - git config branch.main.mergeoptions 'echo \"' && > > - test_must_fail git merge main > > + test_when_finished 'rm -rf repo' && > > + git init repo && > > + ( > > + cd repo && > > + git config alias.split-cmdline-fix 'echo \"' && > > + test_must_fail git split-cmdline-fix && > > + echo foo >foo && > > + git add foo && > > + git commit -m 'initial commit' && > > + git config branch.main.mergeoptions 'echo \"' && > > + test_must_fail git merge main > > + ) > > " > > Maybe, while at it, this test could be modernized to use single quotes > around the test code like: > > test_expect_success 'check split_cmdline return' ' > ... > ' > > or is using double quotes still Ok? In general single quotes are preferable. This test is using quotes internally, which I guess is the reason why we didn't. Happy to change while at it. [snip] > > test_expect_success 'git -c does not split values on equals' ' > > @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' ' > > ' > > > > test_expect_success 'set up custom config file' ' > > - CUSTOM_CONFIG_FILE="custom.conf" && > > - cat >"$CUSTOM_CONFIG_FILE" <<-\EOF > > + cat >"custom.conf" <<-\EOF && > > [user] > > custom = true > > EOF > > + CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path custom.conf)" > > ' > > This looks like a test modernization, but maybe it's part of making > the tests more robust. Anyway it might be a good idea to either talk a > bit about that in the commit message or to move it to a preparatory > commit if it's a modernization and other modernizations could be made > in that preparatory commit. > > Otherwise this patch looks good to me. Yup, this has also been pointed out by others. Will mention in the commit message. Patrick
On Wed, Jan 24, 2024 at 9:52 AM Patrick Steinhardt <ps@pks.im> wrote: > > On Tue, Jan 23, 2024 at 05:15:48PM +0100, Christian Couder wrote: > > On Wed, Jan 10, 2024 at 10:01 AM Patrick Steinhardt <ps@pks.im> wrote: > > > > > > The t1300 test suite exercises the git-config(1) tool. To do so we > > > overwrite ".git/config" to contain custom contents. > > > > Here "we" means "tests in t1300" I guess. > > > > > While this is easy > > > enough to do, it may create problems when using a non-default repository > > > format because we also overwrite the repository format version as well > > > as any potential extensions. > > > > But I am not sure that "we" in the above sentence is also "tests in > > t1300". I think overwriting the repo format version and potential > > extensions is done by other tests, right? Anyway it would be nice to > > clarify this. > > > > > With the upcoming "reftable" ref backend > > > the result is that we may try to access refs via the "files" backend > > > even though the repository has been initialized with the "reftable" > > > backend. > > > > Not sure here also what "we" is. When could refs be accessed via the > > "files" backend even though the repo was initialized with the > > "reftable" backend? > > Yeah, I've rephrased all of these to sey "the tests" or something > similar. > > > Does this mean that some of the tests in t1300 try to access refs via > > the "files" backend while we may want to run all the tests using the > > reftable backend? > > Exactly. We overwrite the ".git/config", which contains the "refStorage" > extension that tells us to use the "reftable" backend. So the extension > is gone, and thus Git would fall back to use the "files" backend > instead, which will fail. Let's take a look at this test: test_expect_success 'check split_cmdline return' " git config alias.split-cmdline-fix 'echo \"' && test_must_fail git split-cmdline-fix && echo foo > foo && git add foo && git commit -m 'initial commit' && git config branch.main.mergeoptions 'echo \"' && test_must_fail git merge main " I don't really see how it overwrites anything. When putting some debug commands before and after that test, it looks like the config file contains the following before that test: --- [section "sub=section"] val1 = foo=bar val2 = foo\nbar val3 = \n\n val4 = val5 [section] val = foo \t bar --- and the following after that test: --- [section "sub=section"] val1 = foo=bar val2 = foo\nbar val3 = \n\n val4 = val5 [section] val = foo \t bar [alias] split-cmdline-fix = echo \" [branch "main"] mergeoptions = echo \" --- So it doesn't look like it overwrites anything. To me it just adds stuff at the end of the file. > > > Refactor tests which access the refdb to be more robust by using their > > > own separate repositories, which allows us to be more careful and not > > > discard required extensions. > > > > Not sure what exactly is discarding extensions. Also robust is not > > very clear. It would be better to give at least an example of how > > things could fail. > > Hm. I don't really know how to phrase this better. The preceding > paragraph already explains why we're discarding the extension and what > the consequence is. I added a sentence saying ", which will cause > failures when trying to access any refs." To me the preceding paragraph said that we are overwriting the config file, but I just don't see how for example the above test overwrites anything. So maybe I am missing something obvious, or maybe you don't quite mean "overwrite", but I don't see how the extension would be discarded by the test which only seems to add stuff.
On Mon, Jan 29, 2024 at 11:32:53AM +0100, Christian Couder wrote: > On Wed, Jan 24, 2024 at 9:52 AM Patrick Steinhardt <ps@pks.im> wrote: > > On Tue, Jan 23, 2024 at 05:15:48PM +0100, Christian Couder wrote: > > > On Wed, Jan 10, 2024 at 10:01 AM Patrick Steinhardt <ps@pks.im> wrote: [snip] > > Hm. I don't really know how to phrase this better. The preceding > > paragraph already explains why we're discarding the extension and what > > the consequence is. I added a sentence saying ", which will cause > > failures when trying to access any refs." > > To me the preceding paragraph said that we are overwriting the config > file, but I just don't see how for example the above test overwrites > anything. So maybe I am missing something obvious, or maybe you don't > quite mean "overwrite", but I don't see how the extension would be > discarded by the test which only seems to add stuff. It happens before already, outside of any tests. See line 1036: ``` cat > .git/config <<\EOF [section "sub=section"] val1 = foo=bar val2 = foo\nbar val3 = \n\n val4 = val5 EOF ``` Overall, I agree that this is rather hard to discover and that the tests really could require a bigger refactoring to make them more independent of each other. I'll send another version that mentions this explicitly. Patrick
diff --git a/t/t1300-config.sh b/t/t1300-config.sh index f4e2752134..53c3d65823 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1099,13 +1099,18 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' ' ' test_expect_success 'check split_cmdline return' " - git config alias.split-cmdline-fix 'echo \"' && - test_must_fail git split-cmdline-fix && - echo foo > foo && - git add foo && - git commit -m 'initial commit' && - git config branch.main.mergeoptions 'echo \"' && - test_must_fail git merge main + test_when_finished 'rm -rf repo' && + git init repo && + ( + cd repo && + git config alias.split-cmdline-fix 'echo \"' && + test_must_fail git split-cmdline-fix && + echo foo >foo && + git add foo && + git commit -m 'initial commit' && + git config branch.main.mergeoptions 'echo \"' && + test_must_fail git merge main + ) " test_expect_success 'git -c "key=value" support' ' @@ -1157,10 +1162,16 @@ test_expect_success 'git -c works with aliases of builtins' ' ' test_expect_success 'aliases can be CamelCased' ' - test_config alias.CamelCased "rev-parse HEAD" && - git CamelCased >out && - git rev-parse HEAD >expect && - test_cmp expect out + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit A && + git config alias.CamelCased "rev-parse HEAD" && + git CamelCased >out && + git rev-parse HEAD >expect && + test_cmp expect out + ) ' test_expect_success 'git -c does not split values on equals' ' @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' ' ' test_expect_success 'set up custom config file' ' - CUSTOM_CONFIG_FILE="custom.conf" && - cat >"$CUSTOM_CONFIG_FILE" <<-\EOF + cat >"custom.conf" <<-\EOF && [user] custom = true EOF + CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path custom.conf)" ' test_expect_success !MINGW 'set up custom config file with special name characters' ' @@ -2052,22 +2063,33 @@ test_expect_success '--show-origin stdin with file include' ' ' test_expect_success '--show-origin blob' ' - blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") && - cat >expect <<-EOF && - blob:$blob user.custom=true - EOF - git config --blob=$blob --show-origin --list >output && - test_cmp expect output + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") && + cat >expect <<-EOF && + blob:$blob user.custom=true + EOF + git config --blob=$blob --show-origin --list >output && + test_cmp expect output + ) ' test_expect_success '--show-origin blob ref' ' - cat >expect <<-\EOF && - blob:main:custom.conf user.custom=true - EOF - git add "$CUSTOM_CONFIG_FILE" && - git commit -m "new config file" && - git config --blob=main:"$CUSTOM_CONFIG_FILE" --show-origin --list >output && - test_cmp expect output + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + cat >expect <<-\EOF && + blob:main:custom.conf user.custom=true + EOF + cp "$CUSTOM_CONFIG_FILE" custom.conf && + git add custom.conf && + git commit -m "new config file" && + git config --blob=main:custom.conf --show-origin --list >output && + test_cmp expect output + ) ' test_expect_success '--show-origin with --default' '
The t1300 test suite exercises the git-config(1) tool. To do so we overwrite ".git/config" to contain custom contents. While this is easy enough to do, it may create problems when using a non-default repository format because we also overwrite the repository format version as well as any potential extensions. With the upcoming "reftable" ref backend the result is that we may try to access refs via the "files" backend even though the repository has been initialized with the "reftable" backend. Refactor tests which access the refdb to be more robust by using their own separate repositories, which allows us to be more careful and not discard required extensions. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- t/t1300-config.sh | 74 ++++++++++++++++++++++++++++++----------------- 1 file changed, 48 insertions(+), 26 deletions(-)