Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()
> From: Junio C Hamano > > +test_expect_success 'git_mkstemps_mode does not fail if fd 0 is not open' ' > > + git init && > > + echo Test. >test-file && > > + git add test-file && > > + git commit -m Message. <&- > > +' > > + > > Yup. I wonder how it would fail without the fix, though ;-) Eh, what? You could run it and see. The test script system will just say "not ok" for this test. If you execute those commands from a shell, you see: $ git init Initialized empty Git repository in /common/not-replicated/worley/temp/1/.git/ $ echo Test. >test-file $ git add test-file $ git commit -m Message. <&- error: unable to create temporary sha1 filename : No such file or directory error: Error building trees $ Dale -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()
wor...@alum.mit.edu (Dale R. Worley) writes: >> From: Junio C Hamano >> >> That's just a plain-vanilla part of POSIX shell behaviour, no? >> >> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07_05 > > "Close standard input" is so weird I never thought it was Posix. In > that case, we can eliminate the C helper program: > > diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh > index 986b2a8..d427f3a 100755 > --- a/t/t0070-fundamental.sh > +++ b/t/t0070-fundamental.sh > @@ -25,6 +25,13 @@ test_expect_success POSIXPERM,SANITY 'mktemp to unwritable > directory prints file > grep "cannotwrite/test" err > ' > > +test_expect_success 'git_mkstemps_mode does not fail if fd 0 is not open' ' > + git init && > + echo Test. >test-file && > + git add test-file && > + git commit -m Message. <&- > +' > + Yup. I wonder how it would fail without the fix, though ;-) > test_expect_success 'check for a bug in the regex routines' ' > # if this test fails, re-build git with NO_REGEX=1 > test-regex > diff --git a/wrapper.c b/wrapper.c > index dd7ecbb..6a015de 100644 > --- a/wrapper.c > +++ b/wrapper.c > @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int > mode) > template[5] = letters[v % num_letters]; v /= num_letters; > > fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode); > - if (fd > 0) > + if (fd >= 0) > return fd; > /* >* Fatal error (EPERM, ENOSPC etc). > > Is this a sensible place to put this test? > > Dale -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()
> From: Junio C Hamano > > That's just a plain-vanilla part of POSIX shell behaviour, no? > > http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07_05 "Close standard input" is so weird I never thought it was Posix. In that case, we can eliminate the C helper program: diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh index 986b2a8..d427f3a 100755 --- a/t/t0070-fundamental.sh +++ b/t/t0070-fundamental.sh @@ -25,6 +25,13 @@ test_expect_success POSIXPERM,SANITY 'mktemp to unwritable directory prints file grep "cannotwrite/test" err ' +test_expect_success 'git_mkstemps_mode does not fail if fd 0 is not open' ' + git init && + echo Test. >test-file && + git add test-file && + git commit -m Message. <&- +' + test_expect_success 'check for a bug in the regex routines' ' # if this test fails, re-build git with NO_REGEX=1 test-regex diff --git a/wrapper.c b/wrapper.c index dd7ecbb..6a015de 100644 --- a/wrapper.c +++ b/wrapper.c @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) template[5] = letters[v % num_letters]; v /= num_letters; fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode); - if (fd > 0) + if (fd >= 0) return fd; /* * Fatal error (EPERM, ENOSPC etc). Is this a sensible place to put this test? Dale -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()
On Thu, Jul 18, 2013 at 1:32 PM, Dale R. Worley wrote: > I've been looking into writing a proper test for this patch. My first > attempt tests the symptom that was seen initially, that "git commit" > fails if fd 0 is closed. > > One problem is how to arrange for fd 0 to be closed. I could use the > bash redirection "<&-", but I think you want to be more portable than > that. That's just a plain-vanilla part of POSIX shell behaviour, no? http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07_05 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()
On Thu, Jul 18, 2013 at 4:32 PM, Dale R. Worley wrote: > I've been looking into writing a proper test for this patch. My first > attempt tests the symptom that was seen initially, that "git commit" > fails if fd 0 is closed. > > One problem is how to arrange for fd 0 to be closed. I could use the > bash redirection "<&-", but I think you want to be more portable than > that. This version uses execvp() inside a small C program, and > execvp() is a Posix function. "<&-" is POSIX. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07_05 > I've tested that this test does what it should: If you remove the > fix, "fd >= 0", the test fails. If you then remove "test-close-fd-0" > from before "git init" in the test, the test is nullified and succeeds > again. > > Here is the diff. What do people think of it? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()
I've been looking into writing a proper test for this patch. My first
attempt tests the symptom that was seen initially, that "git commit"
fails if fd 0 is closed.
One problem is how to arrange for fd 0 to be closed. I could use the
bash redirection "<&-", but I think you want to be more portable than
that. This version uses execvp() inside a small C program, and
execvp() is a Posix function.
I've tested that this test does what it should: If you remove the
fix, "fd >= 0", the test fails. If you then remove "test-close-fd-0"
from before "git init" in the test, the test is nullified and succeeds
again.
Here is the diff. What do people think of it?
diff --git a/Makefile b/Makefile
index 0600eb4..6b410f5 100644
--- a/Makefile
+++ b/Makefile
@@ -557,6 +557,7 @@ X =
PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
TEST_PROGRAMS_NEED_X += test-chmtime
+TEST_PROGRAMS_NEED_X += test-close-fd-0
TEST_PROGRAMS_NEED_X += test-ctype
TEST_PROGRAMS_NEED_X += test-date
TEST_PROGRAMS_NEED_X += test-delta
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 986b2a8..6a31103 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -25,6 +25,13 @@ test_expect_success POSIXPERM,SANITY 'mktemp to unwritable
directory prints file
grep "cannotwrite/test" err
'
+test_expect_success 'git_mkstemps_mode does not fail if fd 0 is not open' '
+ git init &&
+ echo Test. >test-file &&
+ git add test-file &&
+ test-close-fd-0 git commit -m Message.
+'
+
test_expect_success 'check for a bug in the regex routines' '
# if this test fails, re-build git with NO_REGEX=1
test-regex
diff --git a/test-close-fd-0.c b/test-close-fd-0.c
new file mode 100644
index 000..3745c34
--- /dev/null
+++ b/test-close-fd-0.c
@@ -0,0 +1,14 @@
+#include
+
+/* Close file descriptor 0 (which is standard-input), then execute the
+ * remainder of the command line as a command. */
+
+int main(int argc, char **argv)
+{
+ /* Close fd 0. */
+ close(0);
+ /* Execute the requested command. */
+ execvp(argv[1], &argv[1]);
+ /* If execve() failed, return an error. */
+ return 1;
+}
diff --git a/wrapper.c b/wrapper.c
index dd7ecbb..6a015de 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int
mode)
template[5] = letters[v % num_letters]; v /= num_letters;
fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
- if (fd > 0)
+ if (fd >= 0)
return fd;
/*
* Fatal error (EPERM, ENOSPC etc).
Dale
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()
Junio C Hamano writes: > Drew Northup writes: > >> I presume that I should apply this change to my porting of >> git_mkstemps_mode() to tig. If there are no complaints about this for >> a couple of days I will do so. > > Hmph, Thomas and I were actually asking you to give us > > Signed-off-by: Drew Northup Gaahhh, I need a bit more caffeine. Somehow I mixed up Dale and Drew. Sorry for the noise. Please ignore. > for the patch in question. If tig has the same issue, applying that > same patch there may make sense, but that is an independent issue. > > Thanks. > >> >> REF: $gmane/229961 >> >> On 07/17/2013 03:29 PM, Junio C Hamano wrote: >>> Thomas Rast writes: Thomas Rast writes: > From: "Dale R. Worley" > > open() returns -1 on failure, and indeed 0 is a possible success value > if the user closed stdin in our process. Fix the test. > > Signed-off-by: Thomas Rast >> > wrapper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/wrapper.c b/wrapper.c > index dd7ecbb..6a015de 100644 > --- a/wrapper.c > +++ b/wrapper.c > @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, > int mode) > template[5] = letters[v % num_letters]; v /= > num_letters; > > fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode); > - if (fd> 0) > + if (fd>= 0) > return fd; > /* >* Fatal error (EPERM, ENOSPC etc). >> >> >> -- >> -Drew Northup >> -- >> "As opposed to vegetable or mineral error?" >> -John Pescatore, SANS NewsBites Vol. 12 Num. 59 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()
Drew Northup writes: > I presume that I should apply this change to my porting of > git_mkstemps_mode() to tig. If there are no complaints about this for > a couple of days I will do so. Hmph, Thomas and I were actually asking you to give us Signed-off-by: Drew Northup for the patch in question. If tig has the same issue, applying that same patch there may make sense, but that is an independent issue. Thanks. > > REF: $gmane/229961 > > On 07/17/2013 03:29 PM, Junio C Hamano wrote: >> Thomas Rast writes: >>> Thomas Rast writes: From: "Dale R. Worley" open() returns -1 on failure, and indeed 0 is a possible success value if the user closed stdin in our process. Fix the test. Signed-off-by: Thomas Rast > wrapper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wrapper.c b/wrapper.c index dd7ecbb..6a015de 100644 --- a/wrapper.c +++ b/wrapper.c @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) template[5] = letters[v % num_letters]; v /= num_letters; fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode); - if (fd> 0) + if (fd>= 0) return fd; /* * Fatal error (EPERM, ENOSPC etc). > > > -- > -Drew Northup > -- > "As opposed to vegetable or mineral error?" > -John Pescatore, SANS NewsBites Vol. 12 Num. 59 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()
I presume that I should apply this change to my porting of git_mkstemps_mode() to tig. If there are no complaints about this for a couple of days I will do so. REF: $gmane/229961 On 07/17/2013 03:29 PM, Junio C Hamano wrote: Thomas Rast writes: Thomas Rast writes: From: "Dale R. Worley" open() returns -1 on failure, and indeed 0 is a possible success value if the user closed stdin in our process. Fix the test. Signed-off-by: Thomas Rast wrapper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wrapper.c b/wrapper.c index dd7ecbb..6a015de 100644 --- a/wrapper.c +++ b/wrapper.c @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) template[5] = letters[v % num_letters]; v /= num_letters; fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode); - if (fd> 0) + if (fd>= 0) return fd; /* * Fatal error (EPERM, ENOSPC etc). -- -Drew Northup -- "As opposed to vegetable or mineral error?" -John Pescatore, SANS NewsBites Vol. 12 Num. 59 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()
Thomas Rast writes: > Thomas Rast writes: > >> From: "Dale R. Worley" >> >> open() returns -1 on failure, and indeed 0 is a possible success value >> if the user closed stdin in our process. Fix the test. >> >> Signed-off-by: Thomas Rast > > I see you have this in 'pu' without Dale's signoff. I'm guessing > (IANAL) that it's too small to be copyrighted and anyway there is only > way to fix it, but maybe Dale can "sign off" just to be safe, anyway? Yup, that is a good idea. Thanks. > >> wrapper.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/wrapper.c b/wrapper.c >> index dd7ecbb..6a015de 100644 >> --- a/wrapper.c >> +++ b/wrapper.c >> @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int >> mode) >> template[5] = letters[v % num_letters]; v /= num_letters; >> >> fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode); >> -if (fd > 0) >> +if (fd >= 0) >> return fd; >> /* >> * Fatal error (EPERM, ENOSPC etc). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] git_mkstemps: correctly test return value of open()
Thomas Rast writes:
> From: "Dale R. Worley"
>
> open() returns -1 on failure, and indeed 0 is a possible success value
> if the user closed stdin in our process. Fix the test.
>
> Signed-off-by: Thomas Rast
I see you have this in 'pu' without Dale's signoff. I'm guessing
(IANAL) that it's too small to be copyrighted and anyway there is only
way to fix it, but maybe Dale can "sign off" just to be safe, anyway?
> wrapper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/wrapper.c b/wrapper.c
> index dd7ecbb..6a015de 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -322,7 +322,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int
> mode)
> template[5] = letters[v % num_letters]; v /= num_letters;
>
> fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
> - if (fd > 0)
> + if (fd >= 0)
> return fd;
> /*
>* Fatal error (EPERM, ENOSPC etc).
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
