| Message ID | 20240319183722.211300-2-ignacio@iencinas.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add hostname condition to includeIf | expand |
Ignacio Encinas <ignacio@iencinas.com> writes: > diff --git a/Makefile b/Makefile > index 4e255c81f223..561d7a1fa268 100644 > --- a/Makefile > +++ b/Makefile > @@ -863,6 +863,7 @@ TEST_BUILTINS_OBJS += test-userdiff.o > TEST_BUILTINS_OBJS += test-wildmatch.o > TEST_BUILTINS_OBJS += test-windows-named-pipe.o > TEST_BUILTINS_OBJS += test-write-cache.o > +TEST_BUILTINS_OBJS += test-xgethostname.o > TEST_BUILTINS_OBJS += test-xml-encode.o > > # Do not add more tests here unless they have extra dependencies. Add > diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c > index 482a1e58a4b6..9318900a2981 100644 > --- a/t/helper/test-tool.c > +++ b/t/helper/test-tool.c > @@ -86,6 +86,7 @@ static struct test_cmd cmds[] = { > { "truncate", cmd__truncate }, > { "userdiff", cmd__userdiff }, > { "urlmatch-normalization", cmd__urlmatch_normalization }, > + { "xgethostname", cmd__xgethostname }, > { "xml-encode", cmd__xml_encode }, > { "wildmatch", cmd__wildmatch }, > #ifdef GIT_WINDOWS_NATIVE > diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h > index b1be7cfcf593..075d34bd3c0a 100644 > --- a/t/helper/test-tool.h > +++ b/t/helper/test-tool.h > @@ -79,6 +79,7 @@ int cmd__trace2(int argc, const char **argv); > int cmd__truncate(int argc, const char **argv); > int cmd__userdiff(int argc, const char **argv); > int cmd__urlmatch_normalization(int argc, const char **argv); > +int cmd__xgethostname(int argc, const char **argv); > int cmd__xml_encode(int argc, const char **argv); > int cmd__wildmatch(int argc, const char **argv); > #ifdef GIT_WINDOWS_NATIVE > diff --git a/t/helper/test-xgethostname.c b/t/helper/test-xgethostname.c > new file mode 100644 > index 000000000000..285746aef54a > --- /dev/null > +++ b/t/helper/test-xgethostname.c > @@ -0,0 +1,12 @@ > +#include "test-tool.h" > + > +int cmd__xgethostname(int argc, const char **argv) > +{ > + char hostname[HOST_NAME_MAX + 1]; > + > + if (xgethostname(hostname, sizeof(hostname))) > + die("unable to get the host name"); > + > + puts(hostname); > + return 0; > +} OK. Given that xgethostname() is meant as a safe (and improved) replacement for the underlying gethostname(), I would have used gethostname() as the name for the above, but that alone is not big enough reason for another update. > diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh > index 18fe1c25e6a0..613c766e2bb4 100755 > --- a/t/t6500-gc.sh > +++ b/t/t6500-gc.sh > @@ -395,7 +395,6 @@ test_expect_success 'background auto gc respects lock for all operations' ' > > # now fake a concurrent gc that holds the lock; we can use our > # shell pid so that it looks valid. > - hostname=$(hostname || echo unknown) && > shell_pid=$$ && > if test_have_prereq MINGW && test -f /proc/$shell_pid/winpid > then > @@ -404,7 +403,7 @@ test_expect_success 'background auto gc respects lock for all operations' ' > # the Windows PID in this case. > shell_pid=$(cat /proc/$shell_pid/winpid) > fi && > - printf "%d %s" "$shell_pid" "$hostname" >.git/gc.pid && > + printf "%d %s" "$shell_pid" "$(test-tool xgethostname)" >.git/gc.pid && We should replace the "hostname || echo unknown" in the original, instead of doing this change, as it loses the exit status from the "test-tool xgethostname" command. Thanks. > # our gc should exit zero without doing anything > run_and_wait_for_auto_gc &&
On Tue, Mar 19, 2024 at 07:37:21PM +0100, Ignacio Encinas wrote: > diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh > index 18fe1c25e6a0..613c766e2bb4 100755 > --- a/t/t6500-gc.sh > +++ b/t/t6500-gc.sh > @@ -395,7 +395,6 @@ test_expect_success 'background auto gc respects lock for all operations' ' > > # now fake a concurrent gc that holds the lock; we can use our > # shell pid so that it looks valid. > - hostname=$(hostname || echo unknown) && > shell_pid=$$ && > if test_have_prereq MINGW && test -f /proc/$shell_pid/winpid > then > @@ -404,7 +403,7 @@ test_expect_success 'background auto gc respects lock for all operations' ' > # the Windows PID in this case. > shell_pid=$(cat /proc/$shell_pid/winpid) > fi && > - printf "%d %s" "$shell_pid" "$hostname" >.git/gc.pid && > + printf "%d %s" "$shell_pid" "$(test-tool xgethostname)" >.git/gc.pid && Hmm. Before, we were compensating for a failure to run "hostname" by putting in the string "unknown". But I wonder if there were platforms where gethostname() simply fails (e.g., because the hostname is too long). In that case your test-tool returns the empty string, but git-gc internally will use the string "unknown". I think it may be OK, though. The failing exit code from test-tool will be ignored, and we'll end up with a file containing "123 " or similar. Normally we'd use kill() to check that the pid is valid, but with a mis-matched hostname we'll just assume the process is still around, and the outcome is the same. -Peff
On Tue, Mar 19, 2024 at 01:31:06PM -0700, Junio C Hamano wrote: > > diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh > > index 18fe1c25e6a0..613c766e2bb4 100755 > > --- a/t/t6500-gc.sh > > +++ b/t/t6500-gc.sh > > @@ -395,7 +395,6 @@ test_expect_success 'background auto gc respects lock for all operations' ' > > > > # now fake a concurrent gc that holds the lock; we can use our > > # shell pid so that it looks valid. > > - hostname=$(hostname || echo unknown) && > > shell_pid=$$ && > > if test_have_prereq MINGW && test -f /proc/$shell_pid/winpid > > then > > @@ -404,7 +403,7 @@ test_expect_success 'background auto gc respects lock for all operations' ' > > # the Windows PID in this case. > > shell_pid=$(cat /proc/$shell_pid/winpid) > > fi && > > - printf "%d %s" "$shell_pid" "$hostname" >.git/gc.pid && > > + printf "%d %s" "$shell_pid" "$(test-tool xgethostname)" >.git/gc.pid && > > We should replace the "hostname || echo unknown" in the original, > instead of doing this change, as it loses the exit status from the > "test-tool xgethostname" command. I think you need to lose the exit status. Or alternatively do: hostname=$(test-tool xgethostname || echo unknown) See my other reply. -Peff
Jeff King <peff@peff.net> writes: > I think you need to lose the exit status. Or alternatively do: > > hostname=$(test-tool xgethostname || echo unknown) > > See my other reply. As "test-tool xgethostname" runs exactly the same codepath as "includeIf hostname:blah" feature, I would actually prefer for a failing "test-tool gethostname" to _break_ this test so that people can take notice.
On Tue, Mar 19, 2024 at 02:00:34PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > I think you need to lose the exit status. Or alternatively do: > > > > hostname=$(test-tool xgethostname || echo unknown) > > > > See my other reply. > > As "test-tool xgethostname" runs exactly the same codepath as > "includeIf hostname:blah" feature, I would actually prefer for a > failing "test-tool gethostname" to _break_ this test so that people > can take notice. But we are not testing "includeIf" in this patch; we are testing git-gc, which falls back to the string "unknown". The includeIf tests in patch 2 will naturally fail if either xgethostname() fails (because then we will refuse to do any host matching) or if it works but somehow test-tool is broken (because then it won't match what the config code sees internally). -Peff
Jeff King <peff@peff.net> writes: > But we are not testing "includeIf" in this patch; we are testing git-gc, > which falls back to the string "unknown". Ah, I wasn't aware of such a hardcoded default. Then replacing the existing "hostname" with "test-tool xgethostname" and doing nothing else is of course the right thing to do here. Thanks.
On Tue, Mar 19, 2024 at 2:44 PM Junio C Hamano <gitster@pobox.com> wrote: > Jeff King <peff@peff.net> writes: > > > But we are not testing "includeIf" in this patch; we are testing git-gc, > > which falls back to the string "unknown". > > Ah, I wasn't aware of such a hardcoded default. Then replacing the > existing "hostname" with "test-tool xgethostname" and doing nothing > else is of course the right thing to do here. Note that if we choose to use $GIT_HOSTNAME (auto-set-if-unset), we can change the test to simply force $GIT_HOSTNAME to something known to the test script. (That seems orthogonal to this change, but I thought I would mention it.) Chris
diff --git a/Makefile b/Makefile index 4e255c81f223..561d7a1fa268 100644 --- a/Makefile +++ b/Makefile @@ -863,6 +863,7 @@ TEST_BUILTINS_OBJS += test-userdiff.o TEST_BUILTINS_OBJS += test-wildmatch.o TEST_BUILTINS_OBJS += test-windows-named-pipe.o TEST_BUILTINS_OBJS += test-write-cache.o +TEST_BUILTINS_OBJS += test-xgethostname.o TEST_BUILTINS_OBJS += test-xml-encode.o # Do not add more tests here unless they have extra dependencies. Add diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 482a1e58a4b6..9318900a2981 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -86,6 +86,7 @@ static struct test_cmd cmds[] = { { "truncate", cmd__truncate }, { "userdiff", cmd__userdiff }, { "urlmatch-normalization", cmd__urlmatch_normalization }, + { "xgethostname", cmd__xgethostname }, { "xml-encode", cmd__xml_encode }, { "wildmatch", cmd__wildmatch }, #ifdef GIT_WINDOWS_NATIVE diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index b1be7cfcf593..075d34bd3c0a 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -79,6 +79,7 @@ int cmd__trace2(int argc, const char **argv); int cmd__truncate(int argc, const char **argv); int cmd__userdiff(int argc, const char **argv); int cmd__urlmatch_normalization(int argc, const char **argv); +int cmd__xgethostname(int argc, const char **argv); int cmd__xml_encode(int argc, const char **argv); int cmd__wildmatch(int argc, const char **argv); #ifdef GIT_WINDOWS_NATIVE diff --git a/t/helper/test-xgethostname.c b/t/helper/test-xgethostname.c new file mode 100644 index 000000000000..285746aef54a --- /dev/null +++ b/t/helper/test-xgethostname.c @@ -0,0 +1,12 @@ +#include "test-tool.h" + +int cmd__xgethostname(int argc, const char **argv) +{ + char hostname[HOST_NAME_MAX + 1]; + + if (xgethostname(hostname, sizeof(hostname))) + die("unable to get the host name"); + + puts(hostname); + return 0; +} diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh index 18fe1c25e6a0..613c766e2bb4 100755 --- a/t/t6500-gc.sh +++ b/t/t6500-gc.sh @@ -395,7 +395,6 @@ test_expect_success 'background auto gc respects lock for all operations' ' # now fake a concurrent gc that holds the lock; we can use our # shell pid so that it looks valid. - hostname=$(hostname || echo unknown) && shell_pid=$$ && if test_have_prereq MINGW && test -f /proc/$shell_pid/winpid then @@ -404,7 +403,7 @@ test_expect_success 'background auto gc respects lock for all operations' ' # the Windows PID in this case. shell_pid=$(cat /proc/$shell_pid/winpid) fi && - printf "%d %s" "$shell_pid" "$hostname" >.git/gc.pid && + printf "%d %s" "$shell_pid" "$(test-tool xgethostname)" >.git/gc.pid && # our gc should exit zero without doing anything run_and_wait_for_auto_gc &&
Avoid relying on whether the system running the test has "hostname" installed or not, and expose the output of xgethostname through test-tool. Signed-off-by: Ignacio Encinas <ignacio@iencinas.com> --- Makefile | 1 + t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/helper/test-xgethostname.c | 12 ++++++++++++ t/t6500-gc.sh | 3 +-- 5 files changed, 16 insertions(+), 2 deletions(-) create mode 100644 t/helper/test-xgethostname.c