| Message ID | 20221127145130.16155-3-worldhello.net@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/4] t1301: fix wrong template dir for git-init | expand |
Jiang Xin <worldhello.net@gmail.com> writes: > From: Jiang Xin <zhiyou.jx@alibaba-inc.com> > Subject: Re: [PATCH v1 3/4] t1301: wrap the statements in the for loop That makes it sound as if there weren't a loop and now you wrapped the statement in a loop, but that is not what is happening. You are wrapping the statements in something you are not telling us, and "in the for loop" is there only to explain where the statements in question are found. t1301: wrap code to prepare configuration in a separate test or something? > Wrap the statements which were introduced in commit 06cbe85503 (Make > core.sharedRepository more generic, 2008-04-16)) in the for loop in a > new test case, so if we want to skip some of the test cases, these > unwrapped statements won't affect the test cases we choose to run. I am not quite sure why this change is needed for the above, though. If we want to skip u=0660:rw-rw---- test, we can skip the two test_expect_success for the first iteration, which will still run "git config core.sharedrepository" for the first case, and when we test for the next one (i.e. u=0640:rw-r-----), we will overwrite the configuration with the value appropriate for the round. Now you have three separate tests in an interation of the loop. If you skipped the first one in the iteration (by mistake) and let the other two run, they will run with a wrong configuration and values of $x and $y variables, either unset or leftover from the previous round. So I am not sure how this patch can be an improvement. If you wrapped the setting of $x, $y, $u and the config into a helper shell function, e.g. prepare_perm_test_variables () { u=$1 x=... y=... u=... git config core.sharedrepository "$u" } before and outside the loop, and make these two tests in the loop to call it upfront, then your users can skip each test and iteration independently while ensuring that the necessary setup is always done correctly, though. > > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> > --- > t/t1301-shared-repo.sh | 19 ++++++++----------- > 1 file changed, 8 insertions(+), 11 deletions(-) > > diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh > index 1225abbb6d..3ca91bf504 100755 > --- a/t/t1301-shared-repo.sh > +++ b/t/t1301-shared-repo.sh > @@ -78,31 +78,28 @@ for u in 0660:rw-rw---- \ > 0666:rw-rw-rw- \ > 0664:rw-rw-r-- > do > - x=$(expr "$u" : ".*:\([rw-]*\)") && > - y=$(echo "$x" | sed -e "s/w/-/g") && > - u=$(expr "$u" : "\([0-7]*\)") && > - git config core.sharedrepository "$u" && > - umask 0277 && > + test_expect_success POSIXPERM "set variables from $u" ' > + x=$(expr "$u" : ".*:\([rw-]*\)") && > + y=$(echo "$x" | sed -e "s/w/-/g") && > + u=$(expr "$u" : "\([0-7]*\)") && > + git config core.sharedrepository "$u" > + ' > > test_expect_success POSIXPERM "shared = $u ($y) ro" ' > - > + umask 0277 && > rm -f .git/info/refs && > git update-server-info && > actual="$(test_modebits .git/info/refs)" && > verbose test "x$actual" = "x-$y" > - > ' > > - umask 077 && > test_expect_success POSIXPERM "shared = $u ($x) rw" ' > - > + umask 077 && > rm -f .git/info/refs && > git update-server-info && > actual="$(test_modebits .git/info/refs)" && > verbose test "x$actual" = "x-$x" > - > ' > - > done > > test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '
On Mon, Nov 28, 2022 at 12:19 PM Junio C Hamano <gitster@pobox.com> wrote: > Now you have three separate tests in an interation of the loop. If > you skipped the first one in the iteration (by mistake) and let the > other two run, they will run with a wrong configuration and values > of $x and $y variables, either unset or leftover from the previous > round. > > So I am not sure how this patch can be an improvement. I agree that this patch is not that necessary as the other 3 patches and will remove it in next reroll. > > If you wrapped the setting of $x, $y, $u and the config into a > helper shell function, e.g. > > prepare_perm_test_variables () { > u=$1 > x=... > y=... > u=... > git config core.sharedrepository "$u" > } > > before and outside the loop, and make these two tests in the loop to > call it upfront, then your users can skip each test and iteration > independently while ensuring that the necessary setup is always done > correctly, though. >
On Mon, Nov 28, 2022 at 12:19 PM Junio C Hamano <gitster@pobox.com> wrote: > So I am not sure how this patch can be an improvement. > > If you wrapped the setting of $x, $y, $u and the config into a > helper shell function, e.g. > > prepare_perm_test_variables () { > u=$1 > x=... > y=... > u=... > git config core.sharedrepository "$u" > } I tried, but found the first test case passed, but the other test case in the for loop failed. This is because the variable u is changed twice after prepare_perm_test_variables has been called twice. So, will drop this patch 3/4. -- Jiang Xin
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh index 1225abbb6d..3ca91bf504 100755 --- a/t/t1301-shared-repo.sh +++ b/t/t1301-shared-repo.sh @@ -78,31 +78,28 @@ for u in 0660:rw-rw---- \ 0666:rw-rw-rw- \ 0664:rw-rw-r-- do - x=$(expr "$u" : ".*:\([rw-]*\)") && - y=$(echo "$x" | sed -e "s/w/-/g") && - u=$(expr "$u" : "\([0-7]*\)") && - git config core.sharedrepository "$u" && - umask 0277 && + test_expect_success POSIXPERM "set variables from $u" ' + x=$(expr "$u" : ".*:\([rw-]*\)") && + y=$(echo "$x" | sed -e "s/w/-/g") && + u=$(expr "$u" : "\([0-7]*\)") && + git config core.sharedrepository "$u" + ' test_expect_success POSIXPERM "shared = $u ($y) ro" ' - + umask 0277 && rm -f .git/info/refs && git update-server-info && actual="$(test_modebits .git/info/refs)" && verbose test "x$actual" = "x-$y" - ' - umask 077 && test_expect_success POSIXPERM "shared = $u ($x) rw" ' - + umask 077 && rm -f .git/info/refs && git update-server-info && actual="$(test_modebits .git/info/refs)" && verbose test "x$actual" = "x-$x" - ' - done test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '