git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 00/15] Handle fopen() errors
@ 2017-04-20 11:25 Nguyễn Thái Ngọc Duy
  2017-04-20 11:25 ` [PATCH 01/15] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD Nguyễn Thái Ngọc Duy
                   ` (16 more replies)
  0 siblings, 17 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-04-20 11:25 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Nguyễn Thái Ngọc Duy

Some of you may recall a while back, nd/conditional-config-include
failed on Windows because I accidentally fopen()'d a directory in a
test, but it's not considered an serious error unless it's on Windows,
where fopen(<dir>) returns NULL.

A couple of suggestions were thrown back and forth, but I was a bit
busy to follow up. Now that I have time, I have audited all fopen()
calls and try to fix them up for good. There 15 patches, but they only
change one or two lines each. I split them anyway so you can pause
between patches and see if it really makes sense, as changes are all
over the places.

There are still a few iffy fopen() calls in sequencer.c though. I only
fixed the easy ones in there.

The last patch may fail on some platforms since I want to make sure
that fopen(<directory>) == NULL is an expected behavior, even though I
could only test FreeBSD and Linux (and know Windows behaves the same).
At least when people shout up, we could start adding
FREAD_READS_DIRECTORIES on those platforms. That's the goal.

Nguyễn Thái Ngọc Duy (15):
  config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD
  bisect: report on fopen() error
  blame: report error on open if graft_file is a directory
  clone: use xfopen() instead of fopen()
  log: report errno on failure to fopen() a file
  commit.c: report error on failure to fopen() the graft file
  remote.c: report error on failure to fopen()
  rerere.c: report error on failure to fopen()
  rerere.c: report correct errno
  sequencer.c: report error on failure to fopen()
  server-info: report error on failure to fopen()
  wt-status.c: report error on failure to fopen()
  xdiff-interface.c: report errno on failure to stat() or fopen()
  config.c: handle error on failing to fopen()
  t1308: add a test case on open a config directory

 bisect.c              |  5 ++++-
 builtin/blame.c       |  5 ++++-
 builtin/clone.c       |  2 +-
 builtin/log.c         |  3 ++-
 commit.c              |  5 ++++-
 config.c              |  8 +++++++-
 config.mak.uname      |  3 +++
 remote.c              | 12 ++++++++++--
 rerere.c              | 10 +++++++---
 sequencer.c           |  5 ++++-
 server-info.c         |  5 ++++-
 t/t1308-config-set.sh | 13 ++++++++++++-
 wt-status.c           |  2 ++
 xdiff-interface.c     |  4 ++--
 14 files changed, 66 insertions(+), 16 deletions(-)

-- 
2.11.0.157.gd943d85


^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH 01/15] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD
  2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
@ 2017-04-20 11:25 ` Nguyễn Thái Ngọc Duy
  2017-04-20 11:25 ` [PATCH 02/15] bisect: report on fopen() error Nguyễn Thái Ngọc Duy
                   ` (15 subsequent siblings)
  16 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-04-20 11:25 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Nguyễn Thái Ngọc Duy

This variable is added [1] with the assumption that on a sane system,
fopen(<dir>, "r") should return NULL. Linux and FreeBSD do not meet this
expectation while at least Windows and AIX do. Let's make sure they
behave the same way.

I only tested one version on Linux (4.7.0 with glibc 2.22) and
FreeBSD (11.0) but since GNU/kFreeBSD is fbsd kernel with gnu userspace,
I'm pretty sure it shares the same problem.

[1] cba22528fa (Add compat/fopen.c which returns NULL on attempt to open
    directory - 2008-02-08)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 config.mak.uname | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/config.mak.uname b/config.mak.uname
index 399fe19271..a25ffddb3e 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -36,6 +36,7 @@ ifeq ($(uname_S),Linux)
 	NEEDS_LIBRT = YesPlease
 	HAVE_GETDELIM = YesPlease
 	SANE_TEXT_GREP=-a
+	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_ALLOCA_H = YesPlease
@@ -43,6 +44,7 @@ ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_PATHS_H = YesPlease
 	DIR_HAS_BSD_GROUP_SEMANTICS = YesPlease
 	LIBC_CONTAINS_LIBINTL = YesPlease
+	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifeq ($(uname_S),UnixWare)
 	CC = cc
@@ -201,6 +203,7 @@ ifeq ($(uname_S),FreeBSD)
 	GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes
 	HAVE_BSD_SYSCTL = YesPlease
 	PAGER_ENV = LESS=FRX LV=-c MORE=FRX
+	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH 02/15] bisect: report on fopen() error
  2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
  2017-04-20 11:25 ` [PATCH 01/15] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD Nguyễn Thái Ngọc Duy
@ 2017-04-20 11:25 ` Nguyễn Thái Ngọc Duy
  2017-04-20 11:25 ` [PATCH 03/15] blame: report error on open if graft_file is a directory Nguyễn Thái Ngọc Duy
                   ` (14 subsequent siblings)
  16 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-04-20 11:25 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Nguyễn Thái Ngọc Duy

The main thing to catch here is when fopen() is called on a
directory. It's safe even without this change because a few lines
earlier we do check if "filename" is a regular file.

Regardless, let's stay on the safe side in case somebody changes those
lines. Unconditionally printing to stderr by warn_on_inaccessible()
should be fine, because the caller does unconditional fprintf(stderr,
too, no checking for quiet option or anything.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 bisect.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/bisect.c b/bisect.c
index 03af06c66c..6dc4dc9397 100644
--- a/bisect.c
+++ b/bisect.c
@@ -669,8 +669,11 @@ static int is_expected_rev(const struct object_id *oid)
 		return 0;
 
 	fp = fopen(filename, "r");
-	if (!fp)
+	if (!fp) {
+		if (errno != ENOENT)
+			warn_on_inaccessible(filename);
 		return 0;
+	}
 
 	if (strbuf_getline_lf(&str, fp) != EOF)
 		res = !strcmp(str.buf, oid_to_hex(oid));
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH 03/15] blame: report error on open if graft_file is a directory
  2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
  2017-04-20 11:25 ` [PATCH 01/15] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD Nguyễn Thái Ngọc Duy
  2017-04-20 11:25 ` [PATCH 02/15] bisect: report on fopen() error Nguyễn Thái Ngọc Duy
@ 2017-04-20 11:25 ` Nguyễn Thái Ngọc Duy
  2017-04-20 11:25 ` [PATCH 04/15] clone: use xfopen() instead of fopen() Nguyễn Thái Ngọc Duy
                   ` (13 subsequent siblings)
  16 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-04-20 11:25 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/blame.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 07506a3e45..70afa1b05c 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2073,8 +2073,11 @@ static int read_ancestry(const char *graft_file)
 {
 	FILE *fp = fopen(graft_file, "r");
 	struct strbuf buf = STRBUF_INIT;
-	if (!fp)
+	if (!fp) {
+		if (errno != ENOENT)
+			warn_on_inaccessible(graft_file);
 		return -1;
+	}
 	while (!strbuf_getwholeline(&buf, fp, '\n')) {
 		/* The format is just "Commit Parent1 Parent2 ...\n" */
 		struct commit_graft *graft = read_graft_line(buf.buf, buf.len);
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH 04/15] clone: use xfopen() instead of fopen()
  2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
                   ` (2 preceding siblings ...)
  2017-04-20 11:25 ` [PATCH 03/15] blame: report error on open if graft_file is a directory Nguyễn Thái Ngọc Duy
@ 2017-04-20 11:25 ` Nguyễn Thái Ngọc Duy
  2017-04-20 11:25 ` [PATCH 05/15] log: report errno on failure to fopen() a file Nguyễn Thái Ngọc Duy
                   ` (12 subsequent siblings)
  16 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-04-20 11:25 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Nguyễn Thái Ngọc Duy

This code uses the result FILE pointer right away without the NULL
check. Let's use xfopen() and die if we could not open the file. That's
still better than crashing,

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index de85b85254..dde4fe73af 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -357,7 +357,7 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,
 	 * to turn entries with paths relative to the original
 	 * absolute, so that they can be used in the new repository.
 	 */
-	FILE *in = fopen(src->buf, "r");
+	FILE *in = xfopen(src->buf, "r");
 	struct strbuf line = STRBUF_INIT;
 
 	while (strbuf_getline(&line, in) != EOF) {
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH 05/15] log: report errno on failure to fopen() a file
  2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
                   ` (3 preceding siblings ...)
  2017-04-20 11:25 ` [PATCH 04/15] clone: use xfopen() instead of fopen() Nguyễn Thái Ngọc Duy
@ 2017-04-20 11:25 ` Nguyễn Thái Ngọc Duy
  2017-04-21  6:33   ` Jeff King
  2017-04-20 11:26 ` [PATCH 06/15] commit.c: report error on failure to fopen() the graft file Nguyễn Thái Ngọc Duy
                   ` (11 subsequent siblings)
  16 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-04-20 11:25 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/log.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index b3b10cc1ed..26d6a3cf14 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -858,7 +858,8 @@ static int open_next_file(struct commit *commit, const char *subject,
 		printf("%s\n", filename.buf + outdir_offset);
 
 	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
-		return error(_("Cannot open patch file %s"), filename.buf);
+		return error_errno(_("Cannot open patch file %s"),
+				   filename.buf);
 
 	strbuf_release(&filename);
 	return 0;
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH 06/15] commit.c: report error on failure to fopen() the graft file
  2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
                   ` (4 preceding siblings ...)
  2017-04-20 11:25 ` [PATCH 05/15] log: report errno on failure to fopen() a file Nguyễn Thái Ngọc Duy
@ 2017-04-20 11:26 ` Nguyễn Thái Ngọc Duy
  2017-04-20 11:26 ` [PATCH 07/15] remote.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
                   ` (10 subsequent siblings)
  16 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-04-20 11:26 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Nguyễn Thái Ngọc Duy

Suppressing the error (because the command requires --quiet) is not a
concern because we already call error() just a couple lines down.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 commit.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/commit.c b/commit.c
index 73c78c2b80..8726225a7c 100644
--- a/commit.c
+++ b/commit.c
@@ -169,8 +169,11 @@ static int read_graft_file(const char *graft_file)
 {
 	FILE *fp = fopen(graft_file, "r");
 	struct strbuf buf = STRBUF_INIT;
-	if (!fp)
+	if (!fp) {
+		if (errno != ENOENT)
+			warn_on_inaccessible(graft_file);
 		return -1;
+	}
 	while (!strbuf_getwholeline(&buf, fp, '\n')) {
 		/* The format is just "Commit Parent1 Parent2 ...\n" */
 		struct commit_graft *graft = read_graft_line(buf.buf, buf.len);
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH 07/15] remote.c: report error on failure to fopen()
  2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
                   ` (5 preceding siblings ...)
  2017-04-20 11:26 ` [PATCH 06/15] commit.c: report error on failure to fopen() the graft file Nguyễn Thái Ngọc Duy
@ 2017-04-20 11:26 ` Nguyễn Thái Ngọc Duy
  2017-04-26 16:59   ` Johannes Sixt
  2017-04-20 11:26 ` [PATCH 08/15] rerere.c: " Nguyễn Thái Ngọc Duy
                   ` (9 subsequent siblings)
  16 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-04-20 11:26 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Nguyễn Thái Ngọc Duy

There's plenty of error() in this code to safely assume --quiet is not a
concern.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 remote.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index 801137c72e..8ccc1e7b99 100644
--- a/remote.c
+++ b/remote.c
@@ -253,8 +253,12 @@ static void read_remotes_file(struct remote *remote)
 	struct strbuf buf = STRBUF_INIT;
 	FILE *f = fopen(git_path("remotes/%s", remote->name), "r");
 
-	if (!f)
+	if (!f) {
+		if (errno != ENOENT)
+			warn_on_inaccessible(git_path("remotes/%s",
+						      remote->name));
 		return;
+	}
 	remote->configured_in_repo = 1;
 	remote->origin = REMOTE_REMOTES;
 	while (strbuf_getline(&buf, f) != EOF) {
@@ -279,8 +283,12 @@ static void read_branches_file(struct remote *remote)
 	struct strbuf buf = STRBUF_INIT;
 	FILE *f = fopen(git_path("branches/%s", remote->name), "r");
 
-	if (!f)
+	if (!f) {
+		if (errno != ENOENT)
+			warn_on_inaccessible(git_path("branches/%s",
+						      remote->name));
 		return;
+	}
 
 	strbuf_getline_lf(&buf, f);
 	fclose(f);
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH 08/15] rerere.c: report error on failure to fopen()
  2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
                   ` (6 preceding siblings ...)
  2017-04-20 11:26 ` [PATCH 07/15] remote.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
@ 2017-04-20 11:26 ` Nguyễn Thái Ngọc Duy
  2017-04-20 11:26 ` [PATCH 09/15] rerere.c: report correct errno Nguyễn Thái Ngọc Duy
                   ` (8 subsequent siblings)
  16 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-04-20 11:26 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 rerere.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/rerere.c b/rerere.c
index 3bd55caf3b..ef9b11578f 100644
--- a/rerere.c
+++ b/rerere.c
@@ -202,8 +202,11 @@ static void read_rr(struct string_list *rr)
 	struct strbuf buf = STRBUF_INIT;
 	FILE *in = fopen(git_path_merge_rr(), "r");
 
-	if (!in)
+	if (!in) {
+		if (errno != ENOENT)
+			warn_on_inaccessible(git_path_merge_rr());
 		return;
+	}
 	while (!strbuf_getwholeline(&buf, in, '\0')) {
 		char *path;
 		unsigned char sha1[20];
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH 09/15] rerere.c: report correct errno
  2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
                   ` (7 preceding siblings ...)
  2017-04-20 11:26 ` [PATCH 08/15] rerere.c: " Nguyễn Thái Ngọc Duy
@ 2017-04-20 11:26 ` Nguyễn Thái Ngọc Duy
  2017-04-20 11:26 ` [PATCH 10/15] sequencer.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
                   ` (7 subsequent siblings)
  16 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-04-20 11:26 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Nguyễn Thái Ngọc Duy

In the first case, we should have reported the reason fopen() fails. In
the second case, fclose() might change errno but we want to report
fopen() failure.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 rerere.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/rerere.c b/rerere.c
index ef9b11578f..f10c3d8ae6 100644
--- a/rerere.c
+++ b/rerere.c
@@ -487,13 +487,14 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output
 	io.input = fopen(path, "r");
 	io.io.wrerror = 0;
 	if (!io.input)
-		return error("Could not open %s", path);
+		return error_errno("Could not open %s", path);
 
 	if (output) {
 		io.io.output = fopen(output, "w");
 		if (!io.io.output) {
+			error_errno("Could not write %s", output);
 			fclose(io.input);
-			return error("Could not write %s", output);
+			return -1;
 		}
 	}
 
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH 10/15] sequencer.c: report error on failure to fopen()
  2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
                   ` (8 preceding siblings ...)
  2017-04-20 11:26 ` [PATCH 09/15] rerere.c: report correct errno Nguyễn Thái Ngọc Duy
@ 2017-04-20 11:26 ` Nguyễn Thái Ngọc Duy
  2017-04-20 11:26 ` [PATCH 11/15] server-info: " Nguyễn Thái Ngọc Duy
                   ` (6 subsequent siblings)
  16 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-04-20 11:26 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 sequencer.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 77afecaebf..8d5ebfc14f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -919,8 +919,11 @@ static void record_in_rewritten(struct object_id *oid,
 		enum todo_command next_command) {
 	FILE *out = fopen(rebase_path_rewritten_pending(), "a");
 
-	if (!out)
+	if (!out) {
+		if (errno != ENOENT)
+			warn_on_inaccessible(rebase_path_rewritten_pending());
 		return;
+	}
 
 	fprintf(out, "%s\n", oid_to_hex(oid));
 	fclose(out);
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH 11/15] server-info: report error on failure to fopen()
  2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
                   ` (9 preceding siblings ...)
  2017-04-20 11:26 ` [PATCH 10/15] sequencer.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
@ 2017-04-20 11:26 ` Nguyễn Thái Ngọc Duy
  2017-04-20 11:26 ` [PATCH 12/15] wt-status.c: " Nguyễn Thái Ngọc Duy
                   ` (5 subsequent siblings)
  16 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-04-20 11:26 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 server-info.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/server-info.c b/server-info.c
index 7bc4e75d22..7fc2a76966 100644
--- a/server-info.c
+++ b/server-info.c
@@ -132,8 +132,11 @@ static int read_pack_info_file(const char *infofile)
 	int old_cnt = 0;
 
 	fp = fopen(infofile, "r");
-	if (!fp)
+	if (!fp) {
+		if (errno != ENOENT)
+			warn_on_inaccessible(infofile);
 		return 1; /* nonexistent is not an error. */
+	}
 
 	while (fgets(line, sizeof(line), fp)) {
 		int len = strlen(line);
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH 12/15] wt-status.c: report error on failure to fopen()
  2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
                   ` (10 preceding siblings ...)
  2017-04-20 11:26 ` [PATCH 11/15] server-info: " Nguyễn Thái Ngọc Duy
@ 2017-04-20 11:26 ` Nguyễn Thái Ngọc Duy
  2017-04-20 11:26 ` [PATCH 13/15] xdiff-interface.c: report errno on failure to stat() or fopen() Nguyễn Thái Ngọc Duy
                   ` (4 subsequent siblings)
  16 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-04-20 11:26 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 wt-status.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/wt-status.c b/wt-status.c
index 0375484962..5c12bb6ae3 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1067,6 +1067,8 @@ static char *read_line_from_git_path(const char *filename)
 	struct strbuf buf = STRBUF_INIT;
 	FILE *fp = fopen(git_path("%s", filename), "r");
 	if (!fp) {
+		if (errno != ENOENT)
+			warn_on_inaccessible(git_path("%s", filename));
 		strbuf_release(&buf);
 		return NULL;
 	}
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH 13/15] xdiff-interface.c: report errno on failure to stat() or fopen()
  2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
                   ` (11 preceding siblings ...)
  2017-04-20 11:26 ` [PATCH 12/15] wt-status.c: " Nguyễn Thái Ngọc Duy
@ 2017-04-20 11:26 ` Nguyễn Thái Ngọc Duy
  2017-04-20 11:26 ` [PATCH 14/15] config.c: handle error on failing to fopen() Nguyễn Thái Ngọc Duy
                   ` (3 subsequent siblings)
  16 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-04-20 11:26 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 xdiff-interface.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xdiff-interface.c b/xdiff-interface.c
index 060038c2d6..d3f78ca2a7 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -164,9 +164,9 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
 	size_t sz;
 
 	if (stat(filename, &st))
-		return error("Could not stat %s", filename);
+		return error_errno("Could not stat %s", filename);
 	if ((f = fopen(filename, "rb")) == NULL)
-		return error("Could not open %s", filename);
+		return error_errno("Could not open %s", filename);
 	sz = xsize_t(st.st_size);
 	ptr->ptr = xmalloc(sz ? sz : 1);
 	if (sz && fread(ptr->ptr, sz, 1, f) != 1) {
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH 14/15] config.c: handle error on failing to fopen()
  2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
                   ` (12 preceding siblings ...)
  2017-04-20 11:26 ` [PATCH 13/15] xdiff-interface.c: report errno on failure to stat() or fopen() Nguyễn Thái Ngọc Duy
@ 2017-04-20 11:26 ` Nguyễn Thái Ngọc Duy
  2017-04-20 11:26 ` [PATCH 15/15] t1308: add a test case on open a config directory Nguyễn Thái Ngọc Duy
                   ` (2 subsequent siblings)
  16 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-04-20 11:26 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Nguyễn Thái Ngọc Duy

In the first case, we already correctly return -1 if fopen() fails to
open. But we should report something so people know what's wrong.

In the second case, config_file == NULL does not necessarily mean "no
config file". Bail out if needed.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 config.c              | 8 +++++++-
 t/t1308-config-set.sh | 4 +++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 1a4d85537b..ac9effa7aa 100644
--- a/config.c
+++ b/config.c
@@ -1401,7 +1401,8 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 		ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data);
 		funlockfile(f);
 		fclose(f);
-	}
+	} else if (errno != ENOENT)
+		warn_on_inaccessible(filename);
 	return ret;
 }
 
@@ -2601,6 +2602,11 @@ int git_config_rename_section_in_file(const char *config_filename,
 	}
 
 	if (!(config_file = fopen(config_filename, "rb"))) {
+		if (errno != ENOENT) {
+			warn_on_inaccessible(config_filename);
+			ret = -1;
+			goto out;
+		}
 		/* no config file means nothing to rename, no error */
 		goto commit_and_out;
 	}
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index ff50960cca..13e95561f4 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -187,7 +187,9 @@ test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' '
 	chmod -r .git/config &&
 	test_when_finished "chmod +r .git/config" &&
 	echo "Error (-1) reading configuration file .git/config." >expect &&
-	test_expect_code 2 test-config configset_get_value foo.bar .git/config 2>actual &&
+	test_expect_code 2 test-config configset_get_value foo.bar .git/config 2>output &&
+	grep "^warning:" output &&
+	grep "^Error" output >actual &&
 	test_cmp expect actual
 '
 
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH 15/15] t1308: add a test case on open a config directory
  2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
                   ` (13 preceding siblings ...)
  2017-04-20 11:26 ` [PATCH 14/15] config.c: handle error on failing to fopen() Nguyễn Thái Ngọc Duy
@ 2017-04-20 11:26 ` Nguyễn Thái Ngọc Duy
  2017-04-21  1:47 ` [PATCH 00/15] Handle fopen() errors Junio C Hamano
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
  16 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-04-20 11:26 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Nguyễn Thái Ngọc Duy

We don't support config-as-a-directory (maybe someday we will?). Make
sure we consistently fail in this case, which should happen on platforms
where fopen(<directory>) returns non-NULL if FREAD_READS_DIRECTORIES is
defined.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t1308-config-set.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 13e95561f4..e495a61616 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -183,6 +183,15 @@ test_expect_success 'proper error on non-existent files' '
 	test_cmp expect actual
 '
 
+test_expect_success 'proper error on directory "files"' '
+	echo "Error (-1) reading configuration file a-directory." >expect &&
+	mkdir a-directory &&
+	test_expect_code 2 test-config configset_get_value foo.bar a-directory 2>output &&
+	grep "^warning:" output &&
+	grep "^Error" output >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' '
 	chmod -r .git/config &&
 	test_when_finished "chmod +r .git/config" &&
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* Re: [PATCH 00/15] Handle fopen() errors
  2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
                   ` (14 preceding siblings ...)
  2017-04-20 11:26 ` [PATCH 15/15] t1308: add a test case on open a config directory Nguyễn Thái Ngọc Duy
@ 2017-04-21  1:47 ` Junio C Hamano
  2017-04-21  3:41   ` Junio C Hamano
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
  16 siblings, 1 reply; 70+ messages in thread
From: Junio C Hamano @ 2017-04-21  1:47 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King, Johannes Schindelin

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Some of you may recall a while back, nd/conditional-config-include
> failed on Windows because I accidentally fopen()'d a directory in a
> test, but it's not considered an serious error unless it's on Windows,
> where fopen(<dir>) returns NULL.
>
> A couple of suggestions were thrown back and forth, but I was a bit
> busy to follow up. Now that I have time, I have audited all fopen()
> calls and try to fix them up for good. There 15 patches, but they only
> change one or two lines each. I split them anyway so you can pause
> between patches and see if it really makes sense, as changes are all
> over the places.

Nicely done.  

I wonder if it is OK to only special case ENOENT for !fp cases,
where existing code silently returns.  Perhaps it is trying to read
an optional file, and it returns silently because lack of it is
perfectly OK for the purpose of the code.  Are there cases where
this optional file is inside an optional directory, leading to
ENOTDIR, instead of ENOENT, observed and reported by your check?


^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 00/15] Handle fopen() errors
  2017-04-21  1:47 ` [PATCH 00/15] Handle fopen() errors Junio C Hamano
@ 2017-04-21  3:41   ` Junio C Hamano
  2017-04-21  6:29     ` Jeff King
  0 siblings, 1 reply; 70+ messages in thread
From: Junio C Hamano @ 2017-04-21  3:41 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King, Johannes Schindelin

Junio C Hamano <gitster@pobox.com> writes:

> I wonder if it is OK to only special case ENOENT for !fp cases,
> where existing code silently returns.  Perhaps it is trying to read
> an optional file, and it returns silently because lack of it is
> perfectly OK for the purpose of the code.  Are there cases where
> this optional file is inside an optional directory, leading to
> ENOTDIR, instead of ENOENT, observed and reported by your check?

"git grep -B1 warn_on_inaccessible" is enlightening.  I wonder if we
want to wrap the two lines into a hard to misuse helper function,
something along this line.  Would having this patch as a preparatory
step shrink your series?  The patch count would be the same, but you
wouldn't be writing "if (errno != ENOENT)" lines yourself.

 attr.c            | 3 +--
 git-compat-util.h | 3 +++
 wrapper.c         | 6 ++++++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/attr.c b/attr.c
index 1fcf042b87..f695ded53f 100644
--- a/attr.c
+++ b/attr.c
@@ -373,8 +373,7 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
 	int lineno = 0;
 
 	if (!fp) {
-		if (errno != ENOENT && errno != ENOTDIR)
-			warn_on_inaccessible(path);
+		warn_failure_to_read_open_optional_path(path);
 		return NULL;
 	}
 	res = xcalloc(1, sizeof(*res));
diff --git a/git-compat-util.h b/git-compat-util.h
index 8a4a3f85e7..998366c628 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1094,6 +1094,9 @@ int access_or_die(const char *path, int mode, unsigned flag);
 /* Warn on an inaccessible file that ought to be accessible */
 void warn_on_inaccessible(const char *path);
 
+/* Call the above after fopen/open fails for optional input */
+void warn_failure_to_read_open_optional_path(const char *);
+
 #ifdef GMTIME_UNRELIABLE_ERRORS
 struct tm *git_gmtime(const time_t *);
 struct tm *git_gmtime_r(const time_t *, struct tm *);
diff --git a/wrapper.c b/wrapper.c
index 0542fc7582..172cb9fad6 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -576,6 +576,12 @@ int remove_or_warn(unsigned int mode, const char *file)
 	return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
 }
 
+void warn_failure_to_read_open_optional_path(const char *path)
+{
+	if (errno != ENOENT && errno != ENOTDIR)
+		warn_on_inaccessible(path);
+}
+
 void warn_on_inaccessible(const char *path)
 {
 	warning_errno(_("unable to access '%s'"), path);

^ permalink raw reply related	[flat|nested] 70+ messages in thread

* Re: [PATCH 00/15] Handle fopen() errors
  2017-04-21  3:41   ` Junio C Hamano
@ 2017-04-21  6:29     ` Jeff King
  2017-04-21 11:04       ` Duy Nguyen
  0 siblings, 1 reply; 70+ messages in thread
From: Jeff King @ 2017-04-21  6:29 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Nguyễn Thái Ngọc Duy, git, Johannes Schindelin

On Thu, Apr 20, 2017 at 08:41:32PM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I wonder if it is OK to only special case ENOENT for !fp cases,
> > where existing code silently returns.  Perhaps it is trying to read
> > an optional file, and it returns silently because lack of it is
> > perfectly OK for the purpose of the code.  Are there cases where
> > this optional file is inside an optional directory, leading to
> > ENOTDIR, instead of ENOENT, observed and reported by your check?
> 
> "git grep -B1 warn_on_inaccessible" is enlightening.  I wonder if we
> want to wrap the two lines into a hard to misuse helper function,
> something along this line.  Would having this patch as a preparatory
> step shrink your series?  The patch count would be the same, but you
> wouldn't be writing "if (errno != ENOENT)" lines yourself.

I had a similar thought while reading through it. I think it would be
shorter still with:

  FILE *fopen_or_warn(const char *path, const char *mode)
  {
	FILE *fh = fopen(path, mode);
	if (!fh)
		warn_failure_to_read_open_optional_path(path);
	return fh;
  }

And then quite a few of the patches could just be
s/fopen/fopen_or_warn/.

-Peff

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 05/15] log: report errno on failure to fopen() a file
  2017-04-20 11:25 ` [PATCH 05/15] log: report errno on failure to fopen() a file Nguyễn Thái Ngọc Duy
@ 2017-04-21  6:33   ` Jeff King
  0 siblings, 0 replies; 70+ messages in thread
From: Jeff King @ 2017-04-21  6:33 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy
  Cc: git, Junio C Hamano, Johannes Schindelin

On Thu, Apr 20, 2017 at 06:25:59PM +0700, Nguyễn Thái Ngọc Duy wrote:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/log.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/log.c b/builtin/log.c
> index b3b10cc1ed..26d6a3cf14 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -858,7 +858,8 @@ static int open_next_file(struct commit *commit, const char *subject,
>  		printf("%s\n", filename.buf + outdir_offset);
>  
>  	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
> -		return error(_("Cannot open patch file %s"), filename.buf);
> +		return error_errno(_("Cannot open patch file %s"),
> +				   filename.buf);
>  
>  	strbuf_release(&filename);
>  	return 0;

Not a new problem with your patch, but just looking at the context it
seems clear that "filename" is leaked in the error case.

-Peff

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 00/15] Handle fopen() errors
  2017-04-21  6:29     ` Jeff King
@ 2017-04-21 11:04       ` Duy Nguyen
  2017-04-21 11:52         ` Junio C Hamano
  0 siblings, 1 reply; 70+ messages in thread
From: Duy Nguyen @ 2017-04-21 11:04 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Git Mailing List, Johannes Schindelin

On Fri, Apr 21, 2017 at 1:29 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Apr 20, 2017 at 08:41:32PM -0700, Junio C Hamano wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > I wonder if it is OK to only special case ENOENT for !fp cases,
>> > where existing code silently returns.  Perhaps it is trying to read
>> > an optional file, and it returns silently because lack of it is
>> > perfectly OK for the purpose of the code.  Are there cases where
>> > this optional file is inside an optional directory, leading to
>> > ENOTDIR, instead of ENOENT, observed and reported by your check?
>>
>> "git grep -B1 warn_on_inaccessible" is enlightening.  I wonder if we
>> want to wrap the two lines into a hard to misuse helper function,
>> something along this line.  Would having this patch as a preparatory
>> step shrink your series?  The patch count would be the same, but you
>> wouldn't be writing "if (errno != ENOENT)" lines yourself.
>
> I had a similar thought while reading through it. I think it would be
> shorter still with:
>
>   FILE *fopen_or_warn(const char *path, const char *mode)
>   {
>         FILE *fh = fopen(path, mode);
>         if (!fh)
>                 warn_failure_to_read_open_optional_path(path);
>         return fh;
>   }
>
> And then quite a few of the patches could just be
> s/fopen/fopen_or_warn/.

Jeff.. oh Jeff.. you have made it _way_ too convenient that after a
quick grep at fopen( again, I found a couple more places that I would
have just ignored last time (too much work), but now all I need to do
is Alt-f to the end of fopen and Alt-/ a few times. Too tempting.. :)
-- 
Duy

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 00/15] Handle fopen() errors
  2017-04-21 11:04       ` Duy Nguyen
@ 2017-04-21 11:52         ` Junio C Hamano
  2017-04-21 12:27           ` Duy Nguyen
  0 siblings, 1 reply; 70+ messages in thread
From: Junio C Hamano @ 2017-04-21 11:52 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Jeff King, Git Mailing List, Johannes Schindelin

On Fri, Apr 21, 2017 at 8:04 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Fri, Apr 21, 2017 at 1:29 PM, Jeff King <peff@peff.net> wrote:
>>
>> I had a similar thought while reading through it. I think it would be
>> shorter still with:
>>
>>   FILE *fopen_or_warn(const char *path, const char *mode)
>>   {
>>         FILE *fh = fopen(path, mode);
>>         if (!fh)
>>                 warn_failure_to_read_open_optional_path(path);
>>         return fh;
>>   }
>>
>> And then quite a few of the patches could just be
>> s/fopen/fopen_or_warn/.
>
> Jeff.. oh Jeff.. you have made it _way_ too convenient that after a
> quick grep at fopen( again, I found a couple more places that I would
> have just ignored last time (too much work), but now all I need to do
> is Alt-f to the end of fopen and Alt-/ a few times. Too tempting.. :)

Yes, but (1) we'd need to be careful about --quiet, and (2) we would also need
a wrapper for open(2).

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 00/15] Handle fopen() errors
  2017-04-21 11:52         ` Junio C Hamano
@ 2017-04-21 12:27           ` Duy Nguyen
  2017-04-21 17:07             ` Jeff King
  0 siblings, 1 reply; 70+ messages in thread
From: Duy Nguyen @ 2017-04-21 12:27 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Git Mailing List, Johannes Schindelin

On Fri, Apr 21, 2017 at 6:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Yes, but (1) we'd need to be careful about --quiet

Yeah. It's a real pain point for making changes like this. At some
point we should just have a global (maybe multi-level) quiet flag.
-- 
Duy

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 00/15] Handle fopen() errors
  2017-04-21 12:27           ` Duy Nguyen
@ 2017-04-21 17:07             ` Jeff King
  2017-04-24  0:45               ` Junio C Hamano
  0 siblings, 1 reply; 70+ messages in thread
From: Jeff King @ 2017-04-21 17:07 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Junio C Hamano, Git Mailing List, Johannes Schindelin

On Fri, Apr 21, 2017 at 07:27:20PM +0700, Duy Nguyen wrote:

> On Fri, Apr 21, 2017 at 6:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Yes, but (1) we'd need to be careful about --quiet
> 
> Yeah. It's a real pain point for making changes like this. At some
> point we should just have a global (maybe multi-level) quiet flag.

I don't think it's too bad here. Isn't it just:

  FILE *fh = quiet ? fopen(x, y) : fopen_or_warn(x, y);

It is a little annoying to have to repeat "x", though (which is
sometimes a git_path() invocation).

-Peff

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 00/15] Handle fopen() errors
  2017-04-21 17:07             ` Jeff King
@ 2017-04-24  0:45               ` Junio C Hamano
  0 siblings, 0 replies; 70+ messages in thread
From: Junio C Hamano @ 2017-04-24  0:45 UTC (permalink / raw)
  To: Jeff King; +Cc: Duy Nguyen, Git Mailing List, Johannes Schindelin

Jeff King <peff@peff.net> writes:

> On Fri, Apr 21, 2017 at 07:27:20PM +0700, Duy Nguyen wrote:
>
>> On Fri, Apr 21, 2017 at 6:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> > Yes, but (1) we'd need to be careful about --quiet
>> 
>> Yeah. It's a real pain point for making changes like this. At some
>> point we should just have a global (maybe multi-level) quiet flag.
>
> I don't think it's too bad here. Isn't it just:
>
>   FILE *fh = quiet ? fopen(x, y) : fopen_or_warn(x, y);
>
> It is a little annoying to have to repeat "x", though (which is
> sometimes a git_path() invocation).

Sure, but you could do

	fopen_or_warn(quiet, x, y)

if it is a problem ;-)

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 07/15] remote.c: report error on failure to fopen()
  2017-04-20 11:26 ` [PATCH 07/15] remote.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
@ 2017-04-26 16:59   ` Johannes Sixt
  2017-04-27  0:57     ` Junio C Hamano
  0 siblings, 1 reply; 70+ messages in thread
From: Johannes Sixt @ 2017-04-26 16:59 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy
  Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin

Am 20.04.2017 um 13:26 schrieb Nguyễn Thái Ngọc Duy:
> @@ -279,8 +283,12 @@ static void read_branches_file(struct remote *remote)
>  	struct strbuf buf = STRBUF_INIT;
>  	FILE *f = fopen(git_path("branches/%s", remote->name), "r");
>
> -	if (!f)
> +	if (!f) {
> +		if (errno != ENOENT)
> +			warn_on_inaccessible(git_path("branches/%s",
> +						      remote->name));
>  		return;
> +	}
>
>  	strbuf_getline_lf(&buf, f);
>  	fclose(f);

This hunk causes a new failure in t5512.10 'confuses pattern as remote 
when no remote specified' on Windows:

+++ diff -u exp actual
--- exp Wed Apr 26 08:16:10 2017
+++ actual      Wed Apr 26 08:16:10 2017
@@ -1,5 +1,19 @@
+++ case "$1" in
+++ _test_ok=
+++ git ls-remote 'refs*master'
+warning: unable to access '.git/branches/refs*master': Invalid argument
  fatal: 'refs*master' does not appear to be a git repository
  fatal: Could not read from remote repository.

  Please make sure you have the correct access rights
  and the repository exists.
+++ exit_code=128

On Windows, it is not allowed to pass a file name with an asterisk to 
the OS. The test case contains this comment:

# We could just as easily have used "master"; the "*" emphasizes its
# role as a pattern.

So, can we replace the name with a simple "master", our would this miss 
the goal of the test case? Should we make it conditional on the MINGW 
prerequisite?

-- Hannes


^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 07/15] remote.c: report error on failure to fopen()
  2017-04-26 16:59   ` Johannes Sixt
@ 2017-04-27  0:57     ` Junio C Hamano
  2017-04-27  5:07       ` Johannes Sixt
  0 siblings, 1 reply; 70+ messages in thread
From: Junio C Hamano @ 2017-04-27  0:57 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Nguyễn Thái Ngọc Duy, git, Jeff King,
	Johannes Schindelin

Johannes Sixt <j6t@kdbg.org> writes:

> +++ git ls-remote 'refs*master'
> +warning: unable to access '.git/branches/refs*master': Invalid argument
>  fatal: 'refs*master' does not appear to be a git repository
>  fatal: Could not read from remote repository.
>
>  Please make sure you have the correct access rights
>  and the repository exists.
> +++ exit_code=128
>
> On Windows, it is not allowed to pass a file name with an asterisk to
> the OS. The test case contains this comment:
>
> # We could just as easily have used "master"; the "*" emphasizes its
> # role as a pattern.
>
> So, can we replace the name with a simple "master", our would this
> miss the goal of the test case? Should we make it conditional on the
> MINGW prerequisite?

I would actually be more worried about the real-life impact of this
change.  Those who did "git ls-remote 'refs*master'" merely got "it
does not appear to be a git repository" and that was entirely sensible
response from the command.  Can Windows folks live with an extra
warning before it, or do they object to see that new warning?


^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 07/15] remote.c: report error on failure to fopen()
  2017-04-27  0:57     ` Junio C Hamano
@ 2017-04-27  5:07       ` Johannes Sixt
  2017-04-27  9:14         ` Duy Nguyen
  0 siblings, 1 reply; 70+ messages in thread
From: Johannes Sixt @ 2017-04-27  5:07 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Nguyễn Thái Ngọc Duy, git, Jeff King,
	Johannes Schindelin

Am 27.04.2017 um 02:57 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> +++ git ls-remote 'refs*master'
>> +warning: unable to access '.git/branches/refs*master': Invalid argument
>>  fatal: 'refs*master' does not appear to be a git repository
>>  fatal: Could not read from remote repository.
>>
>>  Please make sure you have the correct access rights
>>  and the repository exists.
>> +++ exit_code=128
>>
>> On Windows, it is not allowed to pass a file name with an asterisk to
>> the OS. The test case contains this comment:
>>
>> # We could just as easily have used "master"; the "*" emphasizes its
>> # role as a pattern.
>>
>> So, can we replace the name with a simple "master", our would this
>> miss the goal of the test case? Should we make it conditional on the
>> MINGW prerequisite?
>
> I would actually be more worried about the real-life impact of this
> change.  Those who did "git ls-remote 'refs*master'" merely got "it
> does not appear to be a git repository" and that was entirely sensible
> response from the command.  Can Windows folks live with an extra
> warning before it, or do they object to see that new warning?

I was also worried that the new warning may be irritating. However, I 
expect that it is seen in practice only after a typo. My gut feeling is 
that this is bearable, because the reason for the warning should be obvious.

Unless a use-case turns up where the pattern occurs routinely. We'll 
have to keep the eyes open. Until then it is better to keep the change, IMO.

-- Hannes


^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 07/15] remote.c: report error on failure to fopen()
  2017-04-27  5:07       ` Johannes Sixt
@ 2017-04-27  9:14         ` Duy Nguyen
  2017-04-27 17:49           ` Johannes Sixt
  0 siblings, 1 reply; 70+ messages in thread
From: Duy Nguyen @ 2017-04-27  9:14 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Junio C Hamano, Git Mailing List, Jeff King, Johannes Schindelin

On Thu, Apr 27, 2017 at 12:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 27.04.2017 um 02:57 schrieb Junio C Hamano:
>>
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>>> +++ git ls-remote 'refs*master'
>>> +warning: unable to access '.git/branches/refs*master': Invalid argument
>>>  fatal: 'refs*master' does not appear to be a git repository
>>>  fatal: Could not read from remote repository.
>>>
>>>  Please make sure you have the correct access rights
>>>  and the repository exists.
>>> +++ exit_code=128
>>>
>>> On Windows, it is not allowed to pass a file name with an asterisk to
>>> the OS. The test case contains this comment:
>>>
>>> # We could just as easily have used "master"; the "*" emphasizes its
>>> # role as a pattern.
>>>
>>> So, can we replace the name with a simple "master", our would this
>>> miss the goal of the test case? Should we make it conditional on the
>>> MINGW prerequisite?
>>
>>
>> I would actually be more worried about the real-life impact of this
>> change.  Those who did "git ls-remote 'refs*master'" merely got "it
>> does not appear to be a git repository" and that was entirely sensible
>> response from the command.  Can Windows folks live with an extra
>> warning before it, or do they object to see that new warning?
>
>
> I was also worried that the new warning may be irritating. However, I expect
> that it is seen in practice only after a typo. My gut feeling is that this
> is bearable, because the reason for the warning should be obvious.
>
> Unless a use-case turns up where the pattern occurs routinely. We'll have to
> keep the eyes open. Until then it is better to keep the change, IMO.

OK I'll just add MINGW to the test then. Windows folks can improve
warn_on_inaccessible() to suppress certain class of error messages if
needed.
-- 
Duy

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 07/15] remote.c: report error on failure to fopen()
  2017-04-27  9:14         ` Duy Nguyen
@ 2017-04-27 17:49           ` Johannes Sixt
  0 siblings, 0 replies; 70+ messages in thread
From: Johannes Sixt @ 2017-04-27 17:49 UTC (permalink / raw)
  To: Duy Nguyen
  Cc: Junio C Hamano, Git Mailing List, Jeff King, Johannes Schindelin

Am 27.04.2017 um 11:14 schrieb Duy Nguyen:
> On Thu, Apr 27, 2017 at 12:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> Am 27.04.2017 um 02:57 schrieb Junio C Hamano:
>>>
>>> Johannes Sixt <j6t@kdbg.org> writes:
>>>
>>>> +++ git ls-remote 'refs*master'
>>>> +warning: unable to access '.git/branches/refs*master': Invalid argument
>>>>  fatal: 'refs*master' does not appear to be a git repository
>>>>  fatal: Could not read from remote repository.
>>>>
>>>>  Please make sure you have the correct access rights
>>>>  and the repository exists.
>>>> +++ exit_code=128
>>>>
>>>> On Windows, it is not allowed to pass a file name with an asterisk to
>>>> the OS. The test case contains this comment:
>>>>
>>>> # We could just as easily have used "master"; the "*" emphasizes its
>>>> # role as a pattern.
>>>>
>>>> So, can we replace the name with a simple "master", our would this
>>>> miss the goal of the test case? Should we make it conditional on the
>>>> MINGW prerequisite?

>
> OK I'll just add MINGW to the test then. Windows folks can improve
> warn_on_inaccessible() to suppress certain class of error messages if
> needed.
>

I actually meant something like this:

	if test_have_prereq MINGW
	then
		does_not_exist=master
	else
		does_not_exist="refs*master"
	fi
	test_must_fail git ls-remote "$does_not_exist" >actual 2>&1 &&

-- Hannes


^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH v2 00/21]
  2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
                   ` (15 preceding siblings ...)
  2017-04-21  1:47 ` [PATCH 00/15] Handle fopen() errors Junio C Hamano
@ 2017-05-03 10:16 ` Nguyễn Thái Ngọc Duy
  2017-05-03 10:16   ` [PATCH v2 01/21] Use xfopen() in more places Nguyễn Thái Ngọc Duy
                     ` (21 more replies)
  16 siblings, 22 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

Changes since v1:

 - fopen_or_warn() and warn_on_fopen_errors() are introduced. The
   latter's name is probably not great...
 - A new patch (first one) to convert a bunch to using xfopen()
 - Fix test failure on Windows, found by Johannes Sixt
 - Fix the memory leak in log.c, found by Jeff

There are still a couple of fopen() remained, but I'm getting slow
again so let's get these in first and worry about the rest when
somebody has time.

Nguyễn Thái Ngọc Duy (21):
  Use xfopen() in more places
  clone: use xfopen() instead of fopen()
  config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD
  wrapper.c: add warn_on_fopen_errors()
  wrapper.c: add fopen_or_warn()
  attr.c: use fopen_or_warn()
  ident.c: use fopen_or_warn()
  bisect: report on fopen() error
  blame: report error on open if graft_file is a directory
  log: report errno on failure to fopen() a file
  log: fix memory leak in open_next_file()
  commit.c: report error on failure to fopen() the graft file
  remote.c: report error on failure to fopen()
  rerere.c: report error on failure to fopen()
  rerere.c: report correct errno
  sequencer.c: report error on failure to fopen()
  server-info: report error on failure to fopen()
  wt-status.c: report error on failure to fopen()
  xdiff-interface.c: report errno on failure to stat() or fopen()
  config.c: handle error on failing to fopen()
  t1308: add a test case on open a config directory

 attr.c                |  7 ++-----
 bisect.c              |  7 ++-----
 builtin/am.c          |  8 ++------
 builtin/blame.c       |  4 +++-
 builtin/clone.c       |  2 +-
 builtin/commit.c      |  5 +----
 builtin/fast-export.c |  4 +---
 builtin/fsck.c        |  3 +--
 builtin/log.c         | 11 ++++++++---
 builtin/merge.c       |  4 +---
 builtin/pull.c        |  3 +--
 commit.c              |  4 +++-
 config.c              |  5 ++++-
 config.mak.uname      |  3 +++
 diff.c                |  8 ++------
 dir.c                 |  3 +--
 fast-import.c         |  4 +---
 git-compat-util.h     |  3 +++
 ident.c               |  8 +++-----
 remote-testsvn.c      |  8 ++------
 remote.c              |  4 ++--
 rerere.c              |  7 ++++---
 sequencer.c           |  8 ++++----
 server-info.c         |  2 +-
 t/t1308-config-set.sh | 13 ++++++++++++-
 t/t5512-ls-remote.sh  | 13 ++++++++++---
 wrapper.c             | 21 +++++++++++++++++++++
 wt-status.c           |  3 ++-
 xdiff-interface.c     |  4 ++--
 29 files changed, 103 insertions(+), 76 deletions(-)

-- 
2.11.0.157.gd943d85


^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH v2 01/21] Use xfopen() in more places
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16   ` Nguyễn Thái Ngọc Duy
  2017-05-03 10:16   ` [PATCH v2 02/21] clone: use xfopen() instead of fopen() Nguyễn Thái Ngọc Duy
                     ` (20 subsequent siblings)
  21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

xfopen()

 - provides error details
 - explains error on reading, or writing, or whatever operation
 - has l10n support
 - prints file name in the error

Some of these are missing in the places that are replaced with xfopen(),
which is a clear win. In some other places, it's just less code (not as
clearly a win as the previous case but still is).

The only slight regresssion is in remote-testsvn, where we don't report
the file class (marks files) in the error messages anymore. But since
this is a _test_ svn remote transport, I'm not too concerned.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 bisect.c              | 5 +----
 builtin/am.c          | 8 ++------
 builtin/commit.c      | 5 +----
 builtin/fast-export.c | 4 +---
 builtin/fsck.c        | 3 +--
 builtin/merge.c       | 4 +---
 builtin/pull.c        | 3 +--
 diff.c                | 8 ++------
 fast-import.c         | 4 +---
 remote-testsvn.c      | 8 ++------
 10 files changed, 13 insertions(+), 39 deletions(-)

diff --git a/bisect.c b/bisect.c
index 08c9fb7266..469a3e9061 100644
--- a/bisect.c
+++ b/bisect.c
@@ -438,10 +438,7 @@ static void read_bisect_paths(struct argv_array *array)
 {
 	struct strbuf str = STRBUF_INIT;
 	const char *filename = git_path_bisect_names();
-	FILE *fp = fopen(filename, "r");
-
-	if (!fp)
-		die_errno(_("Could not open file '%s'"), filename);
+	FILE *fp = xfopen(filename, "r");
 
 	while (strbuf_getline_lf(&str, fp) != EOF) {
 		strbuf_trim(&str);
diff --git a/builtin/am.c b/builtin/am.c
index a95dd8b4e6..f5dac7783e 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1275,12 +1275,8 @@ static int parse_mail(struct am_state *state, const char *mail)
 		die("BUG: invalid value for state->scissors");
 	}
 
-	mi.input = fopen(mail, "r");
-	if (!mi.input)
-		die("could not open input");
-	mi.output = fopen(am_path(state, "info"), "w");
-	if (!mi.output)
-		die("could not open output 'info'");
+	mi.input = xfopen(mail, "r");
+	mi.output = xfopen(am_path(state, "info"), "w");
 	if (mailinfo(&mi, am_path(state, "msg"), am_path(state, "patch")))
 		die("could not parse patch");
 
diff --git a/builtin/commit.c b/builtin/commit.c
index 1d805f5da8..eda0d32311 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1695,10 +1695,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		if (!reflog_msg)
 			reflog_msg = "commit (merge)";
 		pptr = commit_list_append(current_head, pptr);
-		fp = fopen(git_path_merge_head(), "r");
-		if (fp == NULL)
-			die_errno(_("could not open '%s' for reading"),
-				  git_path_merge_head());
+		fp = xfopen(git_path_merge_head(), "r");
 		while (strbuf_getline_lf(&m, fp) != EOF) {
 			struct commit *parent;
 
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index e0220630d0..128b99e6da 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -905,9 +905,7 @@ static void export_marks(char *file)
 static void import_marks(char *input_file)
 {
 	char line[512];
-	FILE *f = fopen(input_file, "r");
-	if (!f)
-		die_errno("cannot read '%s'", input_file);
+	FILE *f = xfopen(input_file, "r");
 
 	while (fgets(line, sizeof(line), f)) {
 		uint32_t mark;
diff --git a/builtin/fsck.c b/builtin/fsck.c
index b5e13a4556..00beaaa4e6 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -280,8 +280,7 @@ static void check_unreachable_object(struct object *obj)
 				free(filename);
 				return;
 			}
-			if (!(f = fopen(filename, "w")))
-				die_errno("Could not open '%s'", filename);
+			f = xfopen(filename, "w");
 			if (obj->type == OBJ_BLOB) {
 				if (stream_blob_to_fd(fileno(f), &obj->oid, NULL, 1))
 					die_errno("Could not write '%s'", filename);
diff --git a/builtin/merge.c b/builtin/merge.c
index 703827f006..65a1501858 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -839,9 +839,7 @@ static int suggest_conflicts(void)
 	struct strbuf msgbuf = STRBUF_INIT;
 
 	filename = git_path_merge_msg();
-	fp = fopen(filename, "a");
-	if (!fp)
-		die_errno(_("Could not open '%s' for writing"), filename);
+	fp = xfopen(filename, "a");
 
 	append_conflicts_hint(&msgbuf);
 	fputs(msgbuf.buf, fp);
diff --git a/builtin/pull.c b/builtin/pull.c
index dd1a4a94e4..589c25becf 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -337,8 +337,7 @@ static void get_merge_heads(struct oid_array *merge_heads)
 	struct strbuf sb = STRBUF_INIT;
 	struct object_id oid;
 
-	if (!(fp = fopen(filename, "r")))
-		die_errno(_("could not open '%s' for reading"), filename);
+	fp = xfopen(filename, "r");
 	while (strbuf_getline_lf(&sb, fp) != EOF) {
 		if (get_oid_hex(sb.buf, &oid))
 			continue;  /* invalid line: does not start with SHA1 */
diff --git a/diff.c b/diff.c
index 11eef1c85d..b6597ce568 100644
--- a/diff.c
+++ b/diff.c
@@ -4071,9 +4071,7 @@ int diff_opt_parse(struct diff_options *options,
 		DIFF_OPT_CLR(options, FUNCCONTEXT);
 	else if ((argcount = parse_long_opt("output", av, &optarg))) {
 		char *path = prefix_filename(prefix, optarg);
-		options->file = fopen(path, "w");
-		if (!options->file)
-			die_errno("Could not open '%s'", path);
+		options->file = xfopen(path, "w");
 		options->close_file = 1;
 		if (options->use_color != GIT_COLOR_ALWAYS)
 			options->use_color = GIT_COLOR_NEVER;
@@ -4807,9 +4805,7 @@ void diff_flush(struct diff_options *options)
 		 */
 		if (options->close_file)
 			fclose(options->file);
-		options->file = fopen("/dev/null", "w");
-		if (!options->file)
-			die_errno("Could not open /dev/null");
+		options->file = xfopen("/dev/null", "w");
 		options->close_file = 1;
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
diff --git a/fast-import.c b/fast-import.c
index cf58f875b8..420b3a00d3 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3274,9 +3274,7 @@ static void option_export_pack_edges(const char *edges)
 {
 	if (pack_edges)
 		fclose(pack_edges);
-	pack_edges = fopen(edges, "a");
-	if (!pack_edges)
-		die_errno("Cannot open '%s'", edges);
+	pack_edges = xfopen(edges, "a");
 }
 
 static int parse_one_option(const char *option)
diff --git a/remote-testsvn.c b/remote-testsvn.c
index f87bf851ba..50404ef343 100644
--- a/remote-testsvn.c
+++ b/remote-testsvn.c
@@ -124,10 +124,8 @@ static int note2mark_cb(const unsigned char *object_sha1,
 static void regenerate_marks(void)
 {
 	int ret;
-	FILE *marksfile = fopen(marksfilename, "w+");
+	FILE *marksfile = xfopen(marksfilename, "w+");
 
-	if (!marksfile)
-		die_errno("Couldn't create mark file %s.", marksfilename);
 	ret = for_each_note(NULL, 0, note2mark_cb, marksfile);
 	if (ret)
 		die("Regeneration of marks failed, returned %d.", ret);
@@ -148,9 +146,7 @@ static void check_or_regenerate_marks(int latestrev)
 	marksfile = fopen(marksfilename, "r");
 	if (!marksfile) {
 		regenerate_marks();
-		marksfile = fopen(marksfilename, "r");
-		if (!marksfile)
-			die_errno("cannot read marks file %s!", marksfilename);
+		marksfile = xfopen(marksfilename, "r");
 		fclose(marksfile);
 	} else {
 		strbuf_addf(&sb, ":%d ", latestrev);
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 02/21] clone: use xfopen() instead of fopen()
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
  2017-05-03 10:16   ` [PATCH v2 01/21] Use xfopen() in more places Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16   ` Nguyễn Thái Ngọc Duy
  2017-05-03 15:02     ` Johannes Schindelin
  2017-05-03 10:16   ` [PATCH v2 03/21] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD Nguyễn Thái Ngọc Duy
                     ` (19 subsequent siblings)
  21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

This code uses the result FILE pointer right away without the NULL
check. Let's use xfopen() and die if we could not open the file. That's
still better than crashing,

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index de85b85254..dde4fe73af 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -357,7 +357,7 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,
 	 * to turn entries with paths relative to the original
 	 * absolute, so that they can be used in the new repository.
 	 */
-	FILE *in = fopen(src->buf, "r");
+	FILE *in = xfopen(src->buf, "r");
 	struct strbuf line = STRBUF_INIT;
 
 	while (strbuf_getline(&line, in) != EOF) {
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 03/21] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
  2017-05-03 10:16   ` [PATCH v2 01/21] Use xfopen() in more places Nguyễn Thái Ngọc Duy
  2017-05-03 10:16   ` [PATCH v2 02/21] clone: use xfopen() instead of fopen() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16   ` Nguyễn Thái Ngọc Duy
  2017-05-03 10:16   ` [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors() Nguyễn Thái Ngọc Duy
                     ` (18 subsequent siblings)
  21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

This variable is added [1] with the assumption that on a sane system,
fopen(<dir>, "r") should return NULL. Linux and FreeBSD do not meet this
expectation while at least Windows and AIX do. Let's make sure they
behave the same way.

I only tested one version on Linux (4.7.0 with glibc 2.22) and
FreeBSD (11.0) but since GNU/kFreeBSD is fbsd kernel with gnu userspace,
I'm pretty sure it shares the same problem.

[1] cba22528fa (Add compat/fopen.c which returns NULL on attempt to open
    directory - 2008-02-08)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 config.mak.uname | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/config.mak.uname b/config.mak.uname
index 399fe19271..a25ffddb3e 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -36,6 +36,7 @@ ifeq ($(uname_S),Linux)
 	NEEDS_LIBRT = YesPlease
 	HAVE_GETDELIM = YesPlease
 	SANE_TEXT_GREP=-a
+	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_ALLOCA_H = YesPlease
@@ -43,6 +44,7 @@ ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_PATHS_H = YesPlease
 	DIR_HAS_BSD_GROUP_SEMANTICS = YesPlease
 	LIBC_CONTAINS_LIBINTL = YesPlease
+	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifeq ($(uname_S),UnixWare)
 	CC = cc
@@ -201,6 +203,7 @@ ifeq ($(uname_S),FreeBSD)
 	GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes
 	HAVE_BSD_SYSCTL = YesPlease
 	PAGER_ENV = LESS=FRX LV=-c MORE=FRX
+	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors()
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (2 preceding siblings ...)
  2017-05-03 10:16   ` [PATCH v2 03/21] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16   ` Nguyễn Thái Ngọc Duy
  2017-05-03 15:07     ` Johannes Schindelin
  2017-05-04  5:35     ` Junio C Hamano
  2017-05-03 10:16   ` [PATCH v2 05/21] wrapper.c: add fopen_or_warn() Nguyễn Thái Ngọc Duy
                     ` (17 subsequent siblings)
  21 siblings, 2 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 dir.c             |  3 +--
 git-compat-util.h |  2 ++
 wrapper.c         | 10 ++++++++++
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index f451bfa48c..8218a24962 100644
--- a/dir.c
+++ b/dir.c
@@ -745,8 +745,7 @@ static int add_excludes(const char *fname, const char *base, int baselen,
 
 	fd = open(fname, O_RDONLY);
 	if (fd < 0 || fstat(fd, &st) < 0) {
-		if (errno != ENOENT)
-			warn_on_inaccessible(fname);
+		warn_on_fopen_errors(fname);
 		if (0 <= fd)
 			close(fd);
 		if (!check_index ||
diff --git a/git-compat-util.h b/git-compat-util.h
index bd04564a69..c5b59c23e8 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1099,6 +1099,8 @@ int access_or_die(const char *path, int mode, unsigned flag);
 
 /* Warn on an inaccessible file that ought to be accessible */
 void warn_on_inaccessible(const char *path);
+/* Warn on an inaccessible file if errno indicates this is an error */
+int warn_on_fopen_errors(const char *path);
 
 #ifdef GMTIME_UNRELIABLE_ERRORS
 struct tm *git_gmtime(const time_t *);
diff --git a/wrapper.c b/wrapper.c
index d837417709..20c25e7e65 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -418,6 +418,16 @@ FILE *fopen_for_writing(const char *path)
 	return ret;
 }
 
+int warn_on_fopen_errors(const char *path)
+{
+	if (errno != ENOENT && errno != ENOTDIR) {
+		warn_on_inaccessible(path);
+		return -1;
+	}
+
+	return 0;
+}
+
 int xmkstemp(char *template)
 {
 	int fd;
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 05/21] wrapper.c: add fopen_or_warn()
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (3 preceding siblings ...)
  2017-05-03 10:16   ` [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16   ` Nguyễn Thái Ngọc Duy
  2017-05-03 10:16   ` [PATCH v2 06/21] attr.c: use fopen_or_warn() Nguyễn Thái Ngọc Duy
                     ` (16 subsequent siblings)
  21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

When fopen() returns NULL, it could be because the given path does not
exist, but it could also be some other errors and the caller has to
check. Add a wrapper so we don't have to repeat the same error check
everywhere.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 git-compat-util.h |  1 +
 wrapper.c         | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index c5b59c23e8..c1647b01e0 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -800,6 +800,7 @@ extern int xmkstemp(char *template);
 extern int xmkstemp_mode(char *template, int mode);
 extern char *xgetcwd(void);
 extern FILE *fopen_for_writing(const char *path);
+extern FILE *fopen_or_warn(const char *path, const char *mode);
 
 #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))
diff --git a/wrapper.c b/wrapper.c
index 20c25e7e65..6e513c904a 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -428,6 +428,17 @@ int warn_on_fopen_errors(const char *path)
 	return 0;
 }
 
+FILE *fopen_or_warn(const char *path, const char *mode)
+{
+	FILE *fp = fopen(path, mode);
+
+	if (fp)
+		return fp;
+
+	warn_on_fopen_errors(path);
+	return NULL;
+}
+
 int xmkstemp(char *template)
 {
 	int fd;
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 06/21] attr.c: use fopen_or_warn()
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (4 preceding siblings ...)
  2017-05-03 10:16   ` [PATCH v2 05/21] wrapper.c: add fopen_or_warn() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16   ` Nguyễn Thái Ngọc Duy
  2017-05-03 15:09     ` Johannes Schindelin
  2017-05-03 10:16   ` [PATCH v2 07/21] ident.c: " Nguyễn Thái Ngọc Duy
                     ` (15 subsequent siblings)
  21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 attr.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/attr.c b/attr.c
index 7e2134471c..821203e2a9 100644
--- a/attr.c
+++ b/attr.c
@@ -720,16 +720,13 @@ void git_attr_set_direction(enum git_attr_direction new_direction,
 
 static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
 {
-	FILE *fp = fopen(path, "r");
+	FILE *fp = fopen_or_warn(path, "r");
 	struct attr_stack *res;
 	char buf[2048];
 	int lineno = 0;
 
-	if (!fp) {
-		if (errno != ENOENT && errno != ENOTDIR)
-			warn_on_inaccessible(path);
+	if (!fp)
 		return NULL;
-	}
 	res = xcalloc(1, sizeof(*res));
 	while (fgets(buf, sizeof(buf), fp)) {
 		char *bufp = buf;
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 07/21] ident.c: use fopen_or_warn()
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (5 preceding siblings ...)
  2017-05-03 10:16   ` [PATCH v2 06/21] attr.c: use fopen_or_warn() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16   ` Nguyễn Thái Ngọc Duy
  2017-05-03 10:16   ` [PATCH v2 08/21] bisect: report on fopen() error Nguyễn Thái Ngọc Duy
                     ` (14 subsequent siblings)
  21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 ident.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/ident.c b/ident.c
index bea871c8e0..91c7609055 100644
--- a/ident.c
+++ b/ident.c
@@ -72,12 +72,10 @@ static int add_mailname_host(struct strbuf *buf)
 	FILE *mailname;
 	struct strbuf mailnamebuf = STRBUF_INIT;
 
-	mailname = fopen("/etc/mailname", "r");
-	if (!mailname) {
-		if (errno != ENOENT)
-			warning_errno("cannot open /etc/mailname");
+	mailname = fopen_or_warn("/etc/mailname", "r");
+	if (!mailname)
 		return -1;
-	}
+
 	if (strbuf_getline(&mailnamebuf, mailname) == EOF) {
 		if (ferror(mailname))
 			warning_errno("cannot read /etc/mailname");
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 08/21] bisect: report on fopen() error
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (6 preceding siblings ...)
  2017-05-03 10:16   ` [PATCH v2 07/21] ident.c: " Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16   ` Nguyễn Thái Ngọc Duy
  2017-05-03 15:12     ` Johannes Schindelin
  2017-05-03 10:16   ` [PATCH v2 09/21] blame: report error on open if graft_file is a directory Nguyễn Thái Ngọc Duy
                     ` (13 subsequent siblings)
  21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

The main thing to catch here is when fopen() is called on a
directory. It's safe even without this change because a few lines
earlier we do check if "filename" is a regular file.

Regardless, let's stay on the safe side in case somebody changes those
lines. Unconditionally printing to stderr by warn_on_inaccessible()
should be fine, because the caller does unconditional fprintf(stderr,
too, no checking for quiet option or anything.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 bisect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bisect.c b/bisect.c
index 469a3e9061..bb28bf63b2 100644
--- a/bisect.c
+++ b/bisect.c
@@ -666,7 +666,7 @@ static int is_expected_rev(const struct object_id *oid)
 	if (stat(filename, &st) || !S_ISREG(st.st_mode))
 		return 0;
 
-	fp = fopen(filename, "r");
+	fp = fopen_or_warn(filename, "r");
 	if (!fp)
 		return 0;
 
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 09/21] blame: report error on open if graft_file is a directory
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (7 preceding siblings ...)
  2017-05-03 10:16   ` [PATCH v2 08/21] bisect: report on fopen() error Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16   ` Nguyễn Thái Ngọc Duy
  2017-05-03 15:15     ` Johannes Schindelin
  2017-05-03 10:16   ` [PATCH v2 10/21] log: report errno on failure to fopen() a file Nguyễn Thái Ngọc Duy
                     ` (12 subsequent siblings)
  21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/blame.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 07506a3e45..1648b396dc 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2071,10 +2071,12 @@ static int prepare_lines(struct scoreboard *sb)
  */
 static int read_ancestry(const char *graft_file)
 {
-	FILE *fp = fopen(graft_file, "r");
+	FILE *fp = fopen_or_warn(graft_file, "r");
 	struct strbuf buf = STRBUF_INIT;
+
 	if (!fp)
 		return -1;
+
 	while (!strbuf_getwholeline(&buf, fp, '\n')) {
 		/* The format is just "Commit Parent1 Parent2 ...\n" */
 		struct commit_graft *graft = read_graft_line(buf.buf, buf.len);
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 10/21] log: report errno on failure to fopen() a file
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (8 preceding siblings ...)
  2017-05-03 10:16   ` [PATCH v2 09/21] blame: report error on open if graft_file is a directory Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16   ` Nguyễn Thái Ngọc Duy
  2017-05-03 15:19     ` Johannes Schindelin
  2017-05-03 10:16   ` [PATCH v2 11/21] log: fix memory leak in open_next_file() Nguyễn Thái Ngọc Duy
                     ` (11 subsequent siblings)
  21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/log.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index b3b10cc1ed..26d6a3cf14 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -858,7 +858,8 @@ static int open_next_file(struct commit *commit, const char *subject,
 		printf("%s\n", filename.buf + outdir_offset);
 
 	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
-		return error(_("Cannot open patch file %s"), filename.buf);
+		return error_errno(_("Cannot open patch file %s"),
+				   filename.buf);
 
 	strbuf_release(&filename);
 	return 0;
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 11/21] log: fix memory leak in open_next_file()
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (9 preceding siblings ...)
  2017-05-03 10:16   ` [PATCH v2 10/21] log: report errno on failure to fopen() a file Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16   ` Nguyễn Thái Ngọc Duy
  2017-05-03 10:16   ` [PATCH v2 12/21] commit.c: report error on failure to fopen() the graft file Nguyễn Thái Ngọc Duy
                     ` (10 subsequent siblings)
  21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

Noticed-by: Jeff King <peff@peff.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/log.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 26d6a3cf14..f075838df9 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -842,8 +842,10 @@ static int open_next_file(struct commit *commit, const char *subject,
 	if (output_directory) {
 		strbuf_addstr(&filename, output_directory);
 		if (filename.len >=
-		    PATH_MAX - FORMAT_PATCH_NAME_MAX - suffix_len)
+		    PATH_MAX - FORMAT_PATCH_NAME_MAX - suffix_len) {
+			strbuf_release(&filename);
 			return error(_("name of output directory is too long"));
+		}
 		strbuf_complete(&filename, '/');
 	}
 
@@ -857,9 +859,11 @@ static int open_next_file(struct commit *commit, const char *subject,
 	if (!quiet)
 		printf("%s\n", filename.buf + outdir_offset);
 
-	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
-		return error_errno(_("Cannot open patch file %s"),
-				   filename.buf);
+	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL) {
+		error_errno(_("Cannot open patch file %s"), filename.buf);
+		strbuf_release(&filename);
+		return -1;
+	}
 
 	strbuf_release(&filename);
 	return 0;
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 12/21] commit.c: report error on failure to fopen() the graft file
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (10 preceding siblings ...)
  2017-05-03 10:16   ` [PATCH v2 11/21] log: fix memory leak in open_next_file() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16   ` Nguyễn Thái Ngọc Duy
  2017-05-03 10:16   ` [PATCH v2 13/21] remote.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
                     ` (9 subsequent siblings)
  21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

Suppressing the error (because the command requires --quiet) is not a
concern because we already call error() just a couple lines down.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 commit.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/commit.c b/commit.c
index 73c78c2b80..075607a6da 100644
--- a/commit.c
+++ b/commit.c
@@ -167,10 +167,12 @@ struct commit_graft *read_graft_line(char *buf, int len)
 
 static int read_graft_file(const char *graft_file)
 {
-	FILE *fp = fopen(graft_file, "r");
+	FILE *fp = fopen_or_warn(graft_file, "r");
 	struct strbuf buf = STRBUF_INIT;
+
 	if (!fp)
 		return -1;
+
 	while (!strbuf_getwholeline(&buf, fp, '\n')) {
 		/* The format is just "Commit Parent1 Parent2 ...\n" */
 		struct commit_graft *graft = read_graft_line(buf.buf, buf.len);
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 13/21] remote.c: report error on failure to fopen()
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (11 preceding siblings ...)
  2017-05-03 10:16   ` [PATCH v2 12/21] commit.c: report error on failure to fopen() the graft file Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16   ` Nguyễn Thái Ngọc Duy
  2017-05-03 15:22     ` Johannes Schindelin
  2017-05-03 10:16   ` [PATCH v2 14/21] rerere.c: " Nguyễn Thái Ngọc Duy
                     ` (8 subsequent siblings)
  21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

There's plenty of error() in this code to safely assume --quiet is not a
concern.

t5512 needs update because if we check the path 'refs*master' (i.e. the
asterisk is part of the path) then we'll get an EINVAL error.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 remote.c             |  4 ++--
 t/t5512-ls-remote.sh | 13 ++++++++++---
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index 801137c72e..1f2453d0f6 100644
--- a/remote.c
+++ b/remote.c
@@ -251,7 +251,7 @@ static const char *skip_spaces(const char *s)
 static void read_remotes_file(struct remote *remote)
 {
 	struct strbuf buf = STRBUF_INIT;
-	FILE *f = fopen(git_path("remotes/%s", remote->name), "r");
+	FILE *f = fopen_or_warn(git_path("remotes/%s", remote->name), "r");
 
 	if (!f)
 		return;
@@ -277,7 +277,7 @@ static void read_branches_file(struct remote *remote)
 {
 	char *frag;
 	struct strbuf buf = STRBUF_INIT;
-	FILE *f = fopen(git_path("branches/%s", remote->name), "r");
+	FILE *f = fopen_or_warn(git_path("branches/%s", remote->name), "r");
 
 	if (!f)
 		return;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 94fc9be9ce..02106c9226 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -85,8 +85,15 @@ test_expect_success 'use branch.<name>.remote if possible' '
 '
 
 test_expect_success 'confuses pattern as remote when no remote specified' '
-	cat >exp <<-\EOF &&
-	fatal: '\''refs*master'\'' does not appear to be a git repository
+	if test_have_prereq MINGW
+	then
+		# Windows does not like asterisks in pathname
+		does_not_exist=master
+	else
+		does_not_exist="refs*master"
+	fi &&
+	cat >exp <<-EOF &&
+	fatal: '\''$does_not_exist'\'' does not appear to be a git repository
 	fatal: Could not read from remote repository.
 
 	Please make sure you have the correct access rights
@@ -98,7 +105,7 @@ test_expect_success 'confuses pattern as remote when no remote specified' '
 	# fetch <branch>.
 	# We could just as easily have used "master"; the "*" emphasizes its
 	# role as a pattern.
-	test_must_fail git ls-remote refs*master >actual 2>&1 &&
+	test_must_fail git ls-remote "$does_not_exist" >actual 2>&1 &&
 	test_i18ncmp exp actual
 '
 
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 14/21] rerere.c: report error on failure to fopen()
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (12 preceding siblings ...)
  2017-05-03 10:16   ` [PATCH v2 13/21] remote.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:16   ` Nguyễn Thái Ngọc Duy
  2017-05-03 10:17   ` [PATCH v2 15/21] rerere.c: report correct errno Nguyễn Thái Ngọc Duy
                     ` (7 subsequent siblings)
  21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:16 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 rerere.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rerere.c b/rerere.c
index 3bd55caf3b..971bfedfb2 100644
--- a/rerere.c
+++ b/rerere.c
@@ -200,7 +200,7 @@ static struct rerere_id *new_rerere_id(unsigned char *sha1)
 static void read_rr(struct string_list *rr)
 {
 	struct strbuf buf = STRBUF_INIT;
-	FILE *in = fopen(git_path_merge_rr(), "r");
+	FILE *in = fopen_or_warn(git_path_merge_rr(), "r");
 
 	if (!in)
 		return;
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 15/21] rerere.c: report correct errno
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (13 preceding siblings ...)
  2017-05-03 10:16   ` [PATCH v2 14/21] rerere.c: " Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:17   ` Nguyễn Thái Ngọc Duy
  2017-05-03 15:24     ` Johannes Schindelin
  2017-05-03 10:17   ` [PATCH v2 16/21] sequencer.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
                     ` (6 subsequent siblings)
  21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:17 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

In the first case, we should have reported the reason fopen() fails. In
the second case, fclose() might change errno but we want to report
fopen() failure.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 rerere.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/rerere.c b/rerere.c
index 971bfedfb2..c26c29f87a 100644
--- a/rerere.c
+++ b/rerere.c
@@ -484,13 +484,14 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output
 	io.input = fopen(path, "r");
 	io.io.wrerror = 0;
 	if (!io.input)
-		return error("Could not open %s", path);
+		return error_errno("Could not open %s", path);
 
 	if (output) {
 		io.io.output = fopen(output, "w");
 		if (!io.io.output) {
+			error_errno("Could not write %s", output);
 			fclose(io.input);
-			return error("Could not write %s", output);
+			return -1;
 		}
 	}
 
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 16/21] sequencer.c: report error on failure to fopen()
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (14 preceding siblings ...)
  2017-05-03 10:17   ` [PATCH v2 15/21] rerere.c: report correct errno Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:17   ` Nguyễn Thái Ngọc Duy
  2017-05-03 10:17   ` [PATCH v2 17/21] server-info: " Nguyễn Thái Ngọc Duy
                     ` (5 subsequent siblings)
  21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:17 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 sequencer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 10c3b4ff81..11b5c7c114 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -897,8 +897,8 @@ static void flush_rewritten_pending(void) {
 	FILE *out;
 
 	if (strbuf_read_file(&buf, rebase_path_rewritten_pending(), 82) > 0 &&
-			!get_sha1("HEAD", newsha1) &&
-			(out = fopen(rebase_path_rewritten_list(), "a"))) {
+	    !get_sha1("HEAD", newsha1) &&
+	    (out = fopen_or_warn(rebase_path_rewritten_list(), "a"))) {
 		char *bol = buf.buf, *eol;
 
 		while (*bol) {
@@ -917,7 +917,7 @@ static void flush_rewritten_pending(void) {
 
 static void record_in_rewritten(struct object_id *oid,
 		enum todo_command next_command) {
-	FILE *out = fopen(rebase_path_rewritten_pending(), "a");
+	FILE *out = fopen_or_warn(rebase_path_rewritten_pending(), "a");
 
 	if (!out)
 		return;
@@ -1378,7 +1378,7 @@ static int read_populate_todo(struct todo_list *todo_list,
 
 	if (is_rebase_i(opts)) {
 		struct todo_list done = TODO_LIST_INIT;
-		FILE *f = fopen(rebase_path_msgtotal(), "w");
+		FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
 
 		if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
 				!parse_insn_buffer(done.buf.buf, &done))
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 17/21] server-info: report error on failure to fopen()
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (15 preceding siblings ...)
  2017-05-03 10:17   ` [PATCH v2 16/21] sequencer.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:17   ` Nguyễn Thái Ngọc Duy
  2017-05-03 10:17   ` [PATCH v2 18/21] wt-status.c: " Nguyễn Thái Ngọc Duy
                     ` (4 subsequent siblings)
  21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:17 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 server-info.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server-info.c b/server-info.c
index f6c1a3dfb0..e01ac154a8 100644
--- a/server-info.c
+++ b/server-info.c
@@ -133,7 +133,7 @@ static int read_pack_info_file(const char *infofile)
 	char line[1000];
 	int old_cnt = 0;
 
-	fp = fopen(infofile, "r");
+	fp = fopen_or_warn(infofile, "r");
 	if (!fp)
 		return 1; /* nonexistent is not an error. */
 
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 18/21] wt-status.c: report error on failure to fopen()
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (16 preceding siblings ...)
  2017-05-03 10:17   ` [PATCH v2 17/21] server-info: " Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:17   ` Nguyễn Thái Ngọc Duy
  2017-05-03 10:17   ` [PATCH v2 19/21] xdiff-interface.c: report errno on failure to stat() or fopen() Nguyễn Thái Ngọc Duy
                     ` (3 subsequent siblings)
  21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:17 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 wt-status.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index 0375484962..cdf9f5eed2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1065,7 +1065,8 @@ static void show_am_in_progress(struct wt_status *s,
 static char *read_line_from_git_path(const char *filename)
 {
 	struct strbuf buf = STRBUF_INIT;
-	FILE *fp = fopen(git_path("%s", filename), "r");
+	FILE *fp = fopen_or_warn(git_path("%s", filename), "r");
+
 	if (!fp) {
 		strbuf_release(&buf);
 		return NULL;
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 19/21] xdiff-interface.c: report errno on failure to stat() or fopen()
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (17 preceding siblings ...)
  2017-05-03 10:17   ` [PATCH v2 18/21] wt-status.c: " Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:17   ` Nguyễn Thái Ngọc Duy
  2017-05-03 10:17   ` [PATCH v2 20/21] config.c: handle error on failing to fopen() Nguyễn Thái Ngọc Duy
                     ` (2 subsequent siblings)
  21 siblings, 0 replies; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:17 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 xdiff-interface.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xdiff-interface.c b/xdiff-interface.c
index 060038c2d6..d3f78ca2a7 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -164,9 +164,9 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
 	size_t sz;
 
 	if (stat(filename, &st))
-		return error("Could not stat %s", filename);
+		return error_errno("Could not stat %s", filename);
 	if ((f = fopen(filename, "rb")) == NULL)
-		return error("Could not open %s", filename);
+		return error_errno("Could not open %s", filename);
 	sz = xsize_t(st.st_size);
 	ptr->ptr = xmalloc(sz ? sz : 1);
 	if (sz && fread(ptr->ptr, sz, 1, f) != 1) {
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 20/21] config.c: handle error on failing to fopen()
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (18 preceding siblings ...)
  2017-05-03 10:17   ` [PATCH v2 19/21] xdiff-interface.c: report errno on failure to stat() or fopen() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:17   ` Nguyễn Thái Ngọc Duy
  2017-05-03 15:29     ` Johannes Schindelin
  2017-05-03 10:17   ` [PATCH v2 21/21] t1308: add a test case on open a config directory Nguyễn Thái Ngọc Duy
  2017-05-04  5:45   ` [PATCH v2 00/21] Junio C Hamano
  21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:17 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

In the first case, we already correctly return -1 if fopen() fails to
open. But we should report something so people know what's wrong.

In the second case, config_file == NULL does not necessarily mean "no
config file". Bail out if needed.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 config.c              | 5 ++++-
 t/t1308-config-set.sh | 4 +++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index b4a3205da3..e54d99d519 100644
--- a/config.c
+++ b/config.c
@@ -1422,7 +1422,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 	int ret = -1;
 	FILE *f;
 
-	f = fopen(filename, "r");
+	f = fopen_or_warn(filename, "r");
 	if (f) {
 		flockfile(f);
 		ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data);
@@ -2640,6 +2640,9 @@ int git_config_rename_section_in_file(const char *config_filename,
 	}
 
 	if (!(config_file = fopen(config_filename, "rb"))) {
+		ret = warn_on_fopen_errors(config_filename);
+		if (ret)
+			goto out;
 		/* no config file means nothing to rename, no error */
 		goto commit_and_out;
 	}
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index ff50960cca..13e95561f4 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -187,7 +187,9 @@ test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' '
 	chmod -r .git/config &&
 	test_when_finished "chmod +r .git/config" &&
 	echo "Error (-1) reading configuration file .git/config." >expect &&
-	test_expect_code 2 test-config configset_get_value foo.bar .git/config 2>actual &&
+	test_expect_code 2 test-config configset_get_value foo.bar .git/config 2>output &&
+	grep "^warning:" output &&
+	grep "^Error" output >actual &&
 	test_cmp expect actual
 '
 
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* [PATCH v2 21/21] t1308: add a test case on open a config directory
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (19 preceding siblings ...)
  2017-05-03 10:17   ` [PATCH v2 20/21] config.c: handle error on failing to fopen() Nguyễn Thái Ngọc Duy
@ 2017-05-03 10:17   ` Nguyễn Thái Ngọc Duy
  2017-05-03 15:33     ` Johannes Schindelin
  2017-05-04  5:45   ` [PATCH v2 00/21] Junio C Hamano
  21 siblings, 1 reply; 70+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2017-05-03 10:17 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Johannes Sixt,
	Nguyễn Thái Ngọc Duy

We don't support config-as-a-directory (maybe someday we will?). Make
sure we consistently fail in this case, which should happen on platforms
where fopen(<directory>) returns non-NULL if FREAD_READS_DIRECTORIES is
defined.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t1308-config-set.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 13e95561f4..e495a61616 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -183,6 +183,15 @@ test_expect_success 'proper error on non-existent files' '
 	test_cmp expect actual
 '
 
+test_expect_success 'proper error on directory "files"' '
+	echo "Error (-1) reading configuration file a-directory." >expect &&
+	mkdir a-directory &&
+	test_expect_code 2 test-config configset_get_value foo.bar a-directory 2>output &&
+	grep "^warning:" output &&
+	grep "^Error" output >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' '
 	chmod -r .git/config &&
 	test_when_finished "chmod +r .git/config" &&
-- 
2.11.0.157.gd943d85


^ permalink raw reply related	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 02/21] clone: use xfopen() instead of fopen()
  2017-05-03 10:16   ` [PATCH v2 02/21] clone: use xfopen() instead of fopen() Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:02     ` Johannes Schindelin
  0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:02 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy
  Cc: git, Junio C Hamano, Jeff King, Johannes Sixt

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> This code uses the result FILE pointer right away without the NULL
> check. Let's use xfopen() and die if we could not open the file. That's
> still better than crashing,
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

While funny to read, maybe it would be better to spell it out clearly:

	copy_alternates() called fopen() without handling errors. By
	switching to xfopen(), this bug is fixed.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors()
  2017-05-03 10:16   ` [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors() Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:07     ` Johannes Schindelin
  2017-05-04  5:35     ` Junio C Hamano
  1 sibling, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:07 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy
  Cc: git, Junio C Hamano, Jeff King, Johannes Sixt

[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

This commit message is a bit empty. How about:

	In many places, Git warns about an inaccessible file after a
	fopen() failed. To discern these cases from other cases where we
	want to warn about inaccessible files, introduce a new helper
	specifically to test whether fopen() failed because the current
	user lacks the permission to open file in question.

> diff --git a/dir.c b/dir.c
> index f451bfa48c..8218a24962 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -745,8 +745,7 @@ static int add_excludes(const char *fname, const char *base, int baselen,
>  
>  	fd = open(fname, O_RDONLY);
>  	if (fd < 0 || fstat(fd, &st) < 0) {
> -		if (errno != ENOENT)
> -			warn_on_inaccessible(fname);
> +		warn_on_fopen_errors(fname);
>  		if (0 <= fd)
>  			close(fd);

I see you already converted one location. Why not simply fold all of them
into this patch? It's not like it would be easier to review those changes
if you separate patches addressing this class of problems.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 06/21] attr.c: use fopen_or_warn()
  2017-05-03 10:16   ` [PATCH v2 06/21] attr.c: use fopen_or_warn() Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:09     ` Johannes Schindelin
  0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:09 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy
  Cc: git, Junio C Hamano, Jeff King, Johannes Sixt

[-- Attachment #1: Type: text/plain, Size: 347 bytes --]

Hi Duy,


On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

As commented about warn_on_fopen_errors(), I do not see a benefit in
separating the introduction of this small function from the individual
files' changes to use it. Let's bring it all together.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 08/21] bisect: report on fopen() error
  2017-05-03 10:16   ` [PATCH v2 08/21] bisect: report on fopen() error Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:12     ` Johannes Schindelin
  0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:12 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy
  Cc: git, Junio C Hamano, Jeff King, Johannes Sixt

[-- Attachment #1: Type: text/plain, Size: 867 bytes --]

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> The main thing to catch here is when fopen() is called on a
> directory. It's safe even without this change because a few lines
> earlier we do check if "filename" is a regular file.
> 
> Regardless, let's stay on the safe side in case somebody changes those
> lines. Unconditionally printing to stderr by warn_on_inaccessible()
> should be fine, because the caller does unconditional fprintf(stderr,
> too, no checking for quiet option or anything.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

I appreciate that you mention specifically the use case that triggered my
complaint that in turn prodded you into starting working on this.

However, I think this should be a mere side note in a single commit that
introduces and uses fopen_or_warn().

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 09/21] blame: report error on open if graft_file is a directory
  2017-05-03 10:16   ` [PATCH v2 09/21] blame: report error on open if graft_file is a directory Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:15     ` Johannes Schindelin
  0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:15 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy
  Cc: git, Junio C Hamano, Jeff King, Johannes Sixt

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/blame.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 07506a3e45..1648b396dc 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2071,10 +2071,12 @@ static int prepare_lines(struct scoreboard *sb)
>   */
>  static int read_ancestry(const char *graft_file)
>  {
> -	FILE *fp = fopen(graft_file, "r");
> +	FILE *fp = fopen_or_warn(graft_file, "r");
>  	struct strbuf buf = STRBUF_INIT;
> +
>  	if (!fp)
>  		return -1;
> +
>  	while (!strbuf_getwholeline(&buf, fp, '\n')) {

This is an excellent example to demonstrate why folding all conversions to
fopen_or_warn() into the same patch as introducing that function makes
sense: I had to close this mail, find the mail with the patch introducing
the function, verify that it returns NULL as before, close that mail, and
continue this reply.

Oh, and I do not think it is a good idea to introduce style fixes in the
middle of such a large patch series. It's a bit distracting from the real
meat here.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 10/21] log: report errno on failure to fopen() a file
  2017-05-03 10:16   ` [PATCH v2 10/21] log: report errno on failure to fopen() a file Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:19     ` Johannes Schindelin
  0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:19 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy
  Cc: git, Junio C Hamano, Jeff King, Johannes Sixt

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

I do think that this merits a one-line explanation why you do not use
fopen_or_warn() here.

> diff --git a/builtin/log.c b/builtin/log.c
> index b3b10cc1ed..26d6a3cf14 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -858,7 +858,8 @@ static int open_next_file(struct commit *commit, const char *subject,
>  		printf("%s\n", filename.buf + outdir_offset);
>  
>  	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
> -		return error(_("Cannot open patch file %s"), filename.buf);
> +		return error_errno(_("Cannot open patch file %s"),
> +				   filename.buf);
>  
>  	strbuf_release(&filename);

The patches before and after this one all convert fopen() to
fopen_or_warn(). If you *must* separate those into individual patches
(which just makes this patch series unnecessarily bloated, if you ask me),
you should at least group them a bit better so that the patch series tells
more of a consistent story rather than jumping back and forth like a
hyperactive four-year-old telling you about her latest adventure.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 13/21] remote.c: report error on failure to fopen()
  2017-05-03 10:16   ` [PATCH v2 13/21] remote.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:22     ` Johannes Schindelin
  2017-05-09 10:16       ` Duy Nguyen
  0 siblings, 1 reply; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:22 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy
  Cc: git, Junio C Hamano, Jeff King, Johannes Sixt

[-- Attachment #1: Type: text/plain, Size: 1794 bytes --]

Hi Duy,


On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> There's plenty of error() in this code to safely assume --quiet is not a
> concern.
> 
> t5512 needs update because if we check the path 'refs*master' (i.e. the
> asterisk is part of the path) then we'll get an EINVAL error.

So the first change in this patch unmasks a bug that is fixed by the
second patch?

> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index 94fc9be9ce..02106c9226 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -85,8 +85,15 @@ test_expect_success 'use branch.<name>.remote if possible' '
>  '
>  
>  test_expect_success 'confuses pattern as remote when no remote specified' '
> -	cat >exp <<-\EOF &&
> -	fatal: '\''refs*master'\'' does not appear to be a git repository
> +	if test_have_prereq MINGW
> +	then
> +		# Windows does not like asterisks in pathname
> +		does_not_exist=master
> +	else
> +		does_not_exist="refs*master"
> +	fi &&
> +	cat >exp <<-EOF &&
> +	fatal: '\''$does_not_exist'\'' does not appear to be a git repository
>  	fatal: Could not read from remote repository.
>  
>  	Please make sure you have the correct access rights
> @@ -98,7 +105,7 @@ test_expect_success 'confuses pattern as remote when no remote specified' '
>  	# fetch <branch>.
>  	# We could just as easily have used "master"; the "*" emphasizes its
>  	# role as a pattern.
> -	test_must_fail git ls-remote refs*master >actual 2>&1 &&
> +	test_must_fail git ls-remote "$does_not_exist" >actual 2>&1 &&
>  	test_i18ncmp exp actual
>  '

Sure enough. This totally looks like it needs to be a preparatory bug fix.
Please separate it out and make sure that it comes before the
fopen_or_warn() change in the patch series.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 15/21] rerere.c: report correct errno
  2017-05-03 10:17   ` [PATCH v2 15/21] rerere.c: report correct errno Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:24     ` Johannes Schindelin
  0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:24 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy
  Cc: git, Junio C Hamano, Jeff King, Johannes Sixt

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> diff --git a/rerere.c b/rerere.c
> index 971bfedfb2..c26c29f87a 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -484,13 +484,14 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output
>  	io.input = fopen(path, "r");
>  	io.io.wrerror = 0;
>  	if (!io.input)
> -		return error("Could not open %s", path);
> +		return error_errno("Could not open %s", path);

IMO the error() -> error_errno() changes should all be part of the same
commit, as they probably share the explanation why fopen_or_warn() is not
appropriate here.

>  	if (output) {
>  		io.io.output = fopen(output, "w");
>  		if (!io.io.output) {
> +			error_errno("Could not write %s", output);
>  			fclose(io.input);
> -			return error("Could not write %s", output);
> +			return -1;
>  		}

This one is logically different from the change above, as it not only
cannot be replaced by fopen_or_warn(), but also requires the reordering
due to the different nature of the change.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 20/21] config.c: handle error on failing to fopen()
  2017-05-03 10:17   ` [PATCH v2 20/21] config.c: handle error on failing to fopen() Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:29     ` Johannes Schindelin
  0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:29 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy
  Cc: git, Junio C Hamano, Jeff King, Johannes Sixt

[-- Attachment #1: Type: text/plain, Size: 805 bytes --]

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> In the first case, we already correctly return -1 if fopen() fails to
> open. But we should report something so people know what's wrong.
> 
> In the second case, config_file == NULL does not necessarily mean "no
> config file". Bail out if needed.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Again, it seems that your patch series tries to cut at the file boundary,
not at the "logically the same change" boundary, making it a bit more
cumbersome than necessary to follow this patch series.

I highly recommend squashing the first change into the big fopen() ->
fopen_or_warn() patch I hinted at earlier, and let the second change (with
the accompanying test case) stand on its own.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 21/21] t1308: add a test case on open a config directory
  2017-05-03 10:17   ` [PATCH v2 21/21] t1308: add a test case on open a config directory Nguyễn Thái Ngọc Duy
@ 2017-05-03 15:33     ` Johannes Schindelin
  0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-03 15:33 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy
  Cc: git, Junio C Hamano, Jeff King, Johannes Sixt

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> We don't support config-as-a-directory (maybe someday we will?). Make
> sure we consistently fail in this case, which should happen on platforms
> where fopen(<directory>) returns non-NULL if FREAD_READS_DIRECTORIES is
> defined.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

How about saying that we make sure that this is the case by adding a test
case? Otherwise the reader might (and this reader certainly did) expect a
code change in a .c file.

I reviewed this patch series, and am happy with the overall diff.

My main problem: it should be organized in a more intuitively-graspable
way, i.e. put all the fopen() -> fopen_or_warn() changes into a single
patch, separate out changes (drive-by test fixing for Windows and, ahem,
whitespace changes) into their own commits. Then those commits should be
ordered to relate an easy-flowing story.

Other than that: well done,
Dscho

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors()
  2017-05-03 10:16   ` [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors() Nguyễn Thái Ngọc Duy
  2017-05-03 15:07     ` Johannes Schindelin
@ 2017-05-04  5:35     ` Junio C Hamano
  1 sibling, 0 replies; 70+ messages in thread
From: Junio C Hamano @ 2017-05-04  5:35 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy
  Cc: git, Jeff King, Johannes Schindelin, Johannes Sixt

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  dir.c             |  3 +--
>  git-compat-util.h |  2 ++
>  wrapper.c         | 10 ++++++++++
>  3 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index f451bfa48c..8218a24962 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -745,8 +745,7 @@ static int add_excludes(const char *fname, const char *base, int baselen,
>  
>  	fd = open(fname, O_RDONLY);
>  	if (fd < 0 || fstat(fd, &st) < 0) {
> -		if (errno != ENOENT)
> -			warn_on_inaccessible(fname);
> +		warn_on_fopen_errors(fname);

At least in the original, a reader can guess that, because errno
cannot be NOENT if open(2) succeeded and fstat(2) failed, we call
warn_on_inaccessible() only when we fail to open.

This change makes it harder to see why this is OK when the failure
is not in open(2) but in fstat(2).

	fd = open(fname, O_RDONLY);
	if (fd < 0 || fstat(fd, &st) < 0) {
-		if (errno != ENOENT);
-			warn_on_inaccessible(fname);
-		if (0 <= fd)
+		if (fd < 0)
+			warn_on_fopen_errors(fname);
+		else
			close(fd);
		... and the remainder of the original ...

or something like that, perhaps?

> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -418,6 +418,16 @@ FILE *fopen_for_writing(const char *path)
>  	return ret;
>  }
>  
> +int warn_on_fopen_errors(const char *path)
> +{

Does this need to be returning "int", or should it be "void",
because it always warns when there is an issue, the caller does not
get to choose to give its own warning?

> +	if (errno != ENOENT && errno != ENOTDIR) {
> +		warn_on_inaccessible(path);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  int xmkstemp(char *template)
>  {
>  	int fd;

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 00/21]
  2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
                     ` (20 preceding siblings ...)
  2017-05-03 10:17   ` [PATCH v2 21/21] t1308: add a test case on open a config directory Nguyễn Thái Ngọc Duy
@ 2017-05-04  5:45   ` Junio C Hamano
  2017-05-07  4:20     ` Junio C Hamano
  21 siblings, 1 reply; 70+ messages in thread
From: Junio C Hamano @ 2017-05-04  5:45 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy
  Cc: git, Jeff King, Johannes Schindelin, Johannes Sixt

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Changes since v1:
>
>  - fopen_or_warn() and warn_on_fopen_errors() are introduced. The
>    latter's name is probably not great...
>  - A new patch (first one) to convert a bunch to using xfopen()
>  - Fix test failure on Windows, found by Johannes Sixt
>  - Fix the memory leak in log.c, found by Jeff
>
> There are still a couple of fopen() remained, but I'm getting slow
> again so let's get these in first and worry about the rest when
> somebody has time.
>
> Nguyễn Thái Ngọc Duy (21):
>   Use xfopen() in more places
>   clone: use xfopen() instead of fopen()
>   config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD
>   wrapper.c: add warn_on_fopen_errors()
>   wrapper.c: add fopen_or_warn()
>   attr.c: use fopen_or_warn()
>   ident.c: use fopen_or_warn()
>   bisect: report on fopen() error
>   blame: report error on open if graft_file is a directory
>   log: report errno on failure to fopen() a file
>   log: fix memory leak in open_next_file()
>   commit.c: report error on failure to fopen() the graft file
>   remote.c: report error on failure to fopen()
>   rerere.c: report error on failure to fopen()
>   rerere.c: report correct errno
>   sequencer.c: report error on failure to fopen()
>   server-info: report error on failure to fopen()
>   wt-status.c: report error on failure to fopen()
>   xdiff-interface.c: report errno on failure to stat() or fopen()
>   config.c: handle error on failing to fopen()
>   t1308: add a test case on open a config directory

Thanks.  If the number of parts affected by this series were
smaller, it may have made the review easier to have the introduction
of a helper and its use in a single larger patch, but there are
spread across many, some with files that are touched by different
in-flight topics, and these "collection of smaller patches" makes it
easier to manage both while reviewing and also merging.

All looked good, even though I do share the doubt on the name
"warn-on-fopen-errors"; when something applies equally to fopen(3)
and underlying open(2), I would tend to call that with open not
fopen myself.  But that is a minor point.


^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 00/21]
  2017-05-04  5:45   ` [PATCH v2 00/21] Junio C Hamano
@ 2017-05-07  4:20     ` Junio C Hamano
  2017-05-09 10:22       ` Duy Nguyen
  0 siblings, 1 reply; 70+ messages in thread
From: Junio C Hamano @ 2017-05-07  4:20 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy
  Cc: Git Mailing List, Jeff King, Johannes Schindelin, Johannes Sixt

On Thu, May 4, 2017 at 2:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
> > Changes since v1:
> >
> >  - fopen_or_warn() and warn_on_fopen_errors() are introduced. The
> >    latter's name is probably not great...
> >  - A new patch (first one) to convert a bunch to using xfopen()
> >  - Fix test failure on Windows, found by Johannes Sixt
> >  - Fix the memory leak in log.c, found by Jeff
> >
> > There are still a couple of fopen() remained, but I'm getting slow
> > again so let's get these in first and worry about the rest when
> > somebody has time.

Hmm, is this topic responsible for recent breakage Travis claims on MacOS X?

https://travis-ci.org/git/git/jobs/229585098#L3091

seems to expect an error when test-config is fed a-directory but we are
not getting the expected warning and error?

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 13/21] remote.c: report error on failure to fopen()
  2017-05-03 15:22     ` Johannes Schindelin
@ 2017-05-09 10:16       ` Duy Nguyen
  2017-05-09 12:56         ` Johannes Schindelin
  0 siblings, 1 reply; 70+ messages in thread
From: Duy Nguyen @ 2017-05-09 10:16 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Git Mailing List, Junio C Hamano, Jeff King, Johannes Sixt

Sorry for super late reply. I'm slowly catching up.

On Wed, May 3, 2017 at 10:22 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Duy,
>
>
> On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:
>
>> There's plenty of error() in this code to safely assume --quiet is not a
>> concern.
>>
>> t5512 needs update because if we check the path 'refs*master' (i.e. the
>> asterisk is part of the path) then we'll get an EINVAL error.
>
> So the first change in this patch unmasks a bug that is fixed by the
> second patch?

The change in read_branches_file() in this patch causes the failure.
See the original report [1],

[1] http://public-inbox.org/git/3a2686c2-6766-1235-001f-1b5283b5f408@kdbg.org/
-- 
Duy

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 00/21]
  2017-05-07  4:20     ` Junio C Hamano
@ 2017-05-09 10:22       ` Duy Nguyen
  2017-05-09 22:52         ` Junio C Hamano
  0 siblings, 1 reply; 70+ messages in thread
From: Duy Nguyen @ 2017-05-09 10:22 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Git Mailing List, Jeff King, Johannes Schindelin, Johannes Sixt

On Sun, May 7, 2017 at 11:20 AM, Junio C Hamano <gitster@pobox.com> wrote:
> On Thu, May 4, 2017 at 2:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>> > Changes since v1:
>> >
>> >  - fopen_or_warn() and warn_on_fopen_errors() are introduced. The
>> >    latter's name is probably not great...
>> >  - A new patch (first one) to convert a bunch to using xfopen()
>> >  - Fix test failure on Windows, found by Johannes Sixt
>> >  - Fix the memory leak in log.c, found by Jeff
>> >
>> > There are still a couple of fopen() remained, but I'm getting slow
>> > again so let's get these in first and worry about the rest when
>> > somebody has time.
>
> Hmm, is this topic responsible for recent breakage Travis claims on MacOS X?
>
> https://travis-ci.org/git/git/jobs/229585098#L3091
>
> seems to expect an error when test-config is fed a-directory but we are
> not getting the expected warning and error?

Sounds about right. Let me see if defining FREAD_READS_DIRECTORIES on
MacOS X makes travis happy.
-- 
Duy

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 13/21] remote.c: report error on failure to fopen()
  2017-05-09 10:16       ` Duy Nguyen
@ 2017-05-09 12:56         ` Johannes Schindelin
  0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-05-09 12:56 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Git Mailing List, Junio C Hamano, Jeff King, Johannes Sixt

[-- Attachment #1: Type: text/plain, Size: 1025 bytes --]

Hi Duy,

On Tue, 9 May 2017, Duy Nguyen wrote:

> Sorry for super late reply. I'm slowly catching up.
> 
> On Wed, May 3, 2017 at 10:22 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi Duy,
> >
> >
> > On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:
> >
> >> There's plenty of error() in this code to safely assume --quiet is not a
> >> concern.
> >>
> >> t5512 needs update because if we check the path 'refs*master' (i.e. the
> >> asterisk is part of the path) then we'll get an EINVAL error.
> >
> > So the first change in this patch unmasks a bug that is fixed by the
> > second patch?
> 
> The change in read_branches_file() in this patch causes the failure.
> See the original report [1],
> 
> [1] http://public-inbox.org/git/3a2686c2-6766-1235-001f-1b5283b5f408@kdbg.org/

I disagree. I do not think that the first part of the patch causes the
failure. I think the failure was always there, we just did not bother to
report (and catch) it properly.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 00/21]
  2017-05-09 10:22       ` Duy Nguyen
@ 2017-05-09 22:52         ` Junio C Hamano
  2017-05-10  5:12           ` [PATCH] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too Junio C Hamano
  0 siblings, 1 reply; 70+ messages in thread
From: Junio C Hamano @ 2017-05-09 22:52 UTC (permalink / raw)
  To: Duy Nguyen
  Cc: Git Mailing List, Jeff King, Johannes Schindelin, Johannes Sixt

Duy Nguyen <pclouds@gmail.com> writes:

> On Sun, May 7, 2017 at 11:20 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> On Thu, May 4, 2017 at 2:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>>
>>> > Changes since v1:
>>> >
>>> >  - fopen_or_warn() and warn_on_fopen_errors() are introduced. The
>>> >    latter's name is probably not great...
>>> >  - A new patch (first one) to convert a bunch to using xfopen()
>>> >  - Fix test failure on Windows, found by Johannes Sixt
>>> >  - Fix the memory leak in log.c, found by Jeff
>>> >
>>> > There are still a couple of fopen() remained, but I'm getting slow
>>> > again so let's get these in first and worry about the rest when
>>> > somebody has time.
>>
>> Hmm, is this topic responsible for recent breakage Travis claims on MacOS X?
>>
>> https://travis-ci.org/git/git/jobs/229585098#L3091
>>
>> seems to expect an error when test-config is fed a-directory but we are
>> not getting the expected warning and error?
>
> Sounds about right. Let me see if defining FREAD_READS_DIRECTORIES on
> MacOS X makes travis happy.

Thanks.  I should have suspected that myself to save a round-trip.

^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too
  2017-05-09 22:52         ` Junio C Hamano
@ 2017-05-10  5:12           ` Junio C Hamano
  0 siblings, 0 replies; 70+ messages in thread
From: Junio C Hamano @ 2017-05-10  5:12 UTC (permalink / raw)
  To: Duy Nguyen
  Cc: Git Mailing List, Jeff King, Johannes Schindelin, Johannes Sixt

Let's do the same for Macs, as it is BSD variant after all.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 config.mak.uname | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config.mak.uname b/config.mak.uname
index a25ffddb3e..1743890164 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -110,6 +110,7 @@ ifeq ($(uname_S),Darwin)
 	BASIC_CFLAGS += -DPRECOMPOSE_UNICODE
 	BASIC_CFLAGS += -DPROTECT_HFS_DEFAULT=1
 	HAVE_BSD_SYSCTL = YesPlease
+	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifeq ($(uname_S),SunOS)
 	NEEDS_SOCKET = YesPlease
-- 
2.13.0-336-gf73534b083


^ permalink raw reply related	[flat|nested] 70+ messages in thread

end of thread, other threads:[~2017-05-10  5:12 UTC | newest]

Thread overview: 70+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-20 11:25 [PATCH 00/15] Handle fopen() errors Nguyễn Thái Ngọc Duy
2017-04-20 11:25 ` [PATCH 01/15] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD Nguyễn Thái Ngọc Duy
2017-04-20 11:25 ` [PATCH 02/15] bisect: report on fopen() error Nguyễn Thái Ngọc Duy
2017-04-20 11:25 ` [PATCH 03/15] blame: report error on open if graft_file is a directory Nguyễn Thái Ngọc Duy
2017-04-20 11:25 ` [PATCH 04/15] clone: use xfopen() instead of fopen() Nguyễn Thái Ngọc Duy
2017-04-20 11:25 ` [PATCH 05/15] log: report errno on failure to fopen() a file Nguyễn Thái Ngọc Duy
2017-04-21  6:33   ` Jeff King
2017-04-20 11:26 ` [PATCH 06/15] commit.c: report error on failure to fopen() the graft file Nguyễn Thái Ngọc Duy
2017-04-20 11:26 ` [PATCH 07/15] remote.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
2017-04-26 16:59   ` Johannes Sixt
2017-04-27  0:57     ` Junio C Hamano
2017-04-27  5:07       ` Johannes Sixt
2017-04-27  9:14         ` Duy Nguyen
2017-04-27 17:49           ` Johannes Sixt
2017-04-20 11:26 ` [PATCH 08/15] rerere.c: " Nguyễn Thái Ngọc Duy
2017-04-20 11:26 ` [PATCH 09/15] rerere.c: report correct errno Nguyễn Thái Ngọc Duy
2017-04-20 11:26 ` [PATCH 10/15] sequencer.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
2017-04-20 11:26 ` [PATCH 11/15] server-info: " Nguyễn Thái Ngọc Duy
2017-04-20 11:26 ` [PATCH 12/15] wt-status.c: " Nguyễn Thái Ngọc Duy
2017-04-20 11:26 ` [PATCH 13/15] xdiff-interface.c: report errno on failure to stat() or fopen() Nguyễn Thái Ngọc Duy
2017-04-20 11:26 ` [PATCH 14/15] config.c: handle error on failing to fopen() Nguyễn Thái Ngọc Duy
2017-04-20 11:26 ` [PATCH 15/15] t1308: add a test case on open a config directory Nguyễn Thái Ngọc Duy
2017-04-21  1:47 ` [PATCH 00/15] Handle fopen() errors Junio C Hamano
2017-04-21  3:41   ` Junio C Hamano
2017-04-21  6:29     ` Jeff King
2017-04-21 11:04       ` Duy Nguyen
2017-04-21 11:52         ` Junio C Hamano
2017-04-21 12:27           ` Duy Nguyen
2017-04-21 17:07             ` Jeff King
2017-04-24  0:45               ` Junio C Hamano
2017-05-03 10:16 ` [PATCH v2 00/21] Nguyễn Thái Ngọc Duy
2017-05-03 10:16   ` [PATCH v2 01/21] Use xfopen() in more places Nguyễn Thái Ngọc Duy
2017-05-03 10:16   ` [PATCH v2 02/21] clone: use xfopen() instead of fopen() Nguyễn Thái Ngọc Duy
2017-05-03 15:02     ` Johannes Schindelin
2017-05-03 10:16   ` [PATCH v2 03/21] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD Nguyễn Thái Ngọc Duy
2017-05-03 10:16   ` [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors() Nguyễn Thái Ngọc Duy
2017-05-03 15:07     ` Johannes Schindelin
2017-05-04  5:35     ` Junio C Hamano
2017-05-03 10:16   ` [PATCH v2 05/21] wrapper.c: add fopen_or_warn() Nguyễn Thái Ngọc Duy
2017-05-03 10:16   ` [PATCH v2 06/21] attr.c: use fopen_or_warn() Nguyễn Thái Ngọc Duy
2017-05-03 15:09     ` Johannes Schindelin
2017-05-03 10:16   ` [PATCH v2 07/21] ident.c: " Nguyễn Thái Ngọc Duy
2017-05-03 10:16   ` [PATCH v2 08/21] bisect: report on fopen() error Nguyễn Thái Ngọc Duy
2017-05-03 15:12     ` Johannes Schindelin
2017-05-03 10:16   ` [PATCH v2 09/21] blame: report error on open if graft_file is a directory Nguyễn Thái Ngọc Duy
2017-05-03 15:15     ` Johannes Schindelin
2017-05-03 10:16   ` [PATCH v2 10/21] log: report errno on failure to fopen() a file Nguyễn Thái Ngọc Duy
2017-05-03 15:19     ` Johannes Schindelin
2017-05-03 10:16   ` [PATCH v2 11/21] log: fix memory leak in open_next_file() Nguyễn Thái Ngọc Duy
2017-05-03 10:16   ` [PATCH v2 12/21] commit.c: report error on failure to fopen() the graft file Nguyễn Thái Ngọc Duy
2017-05-03 10:16   ` [PATCH v2 13/21] remote.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
2017-05-03 15:22     ` Johannes Schindelin
2017-05-09 10:16       ` Duy Nguyen
2017-05-09 12:56         ` Johannes Schindelin
2017-05-03 10:16   ` [PATCH v2 14/21] rerere.c: " Nguyễn Thái Ngọc Duy
2017-05-03 10:17   ` [PATCH v2 15/21] rerere.c: report correct errno Nguyễn Thái Ngọc Duy
2017-05-03 15:24     ` Johannes Schindelin
2017-05-03 10:17   ` [PATCH v2 16/21] sequencer.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
2017-05-03 10:17   ` [PATCH v2 17/21] server-info: " Nguyễn Thái Ngọc Duy
2017-05-03 10:17   ` [PATCH v2 18/21] wt-status.c: " Nguyễn Thái Ngọc Duy
2017-05-03 10:17   ` [PATCH v2 19/21] xdiff-interface.c: report errno on failure to stat() or fopen() Nguyễn Thái Ngọc Duy
2017-05-03 10:17   ` [PATCH v2 20/21] config.c: handle error on failing to fopen() Nguyễn Thái Ngọc Duy
2017-05-03 15:29     ` Johannes Schindelin
2017-05-03 10:17   ` [PATCH v2 21/21] t1308: add a test case on open a config directory Nguyễn Thái Ngọc Duy
2017-05-03 15:33     ` Johannes Schindelin
2017-05-04  5:45   ` [PATCH v2 00/21] Junio C Hamano
2017-05-07  4:20     ` Junio C Hamano
2017-05-09 10:22       ` Duy Nguyen
2017-05-09 22:52         ` Junio C Hamano
2017-05-10  5:12           ` [PATCH] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too Junio C Hamano

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).