git@vger.kernel.org mailing list mirror (one of many)
 help / 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
                   ` (15 more replies)
  0 siblings, 16 replies; 30+ 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|threaded] 30+ 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
                   ` (14 subsequent siblings)
  15 siblings, 0 replies; 30+ 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	[flat|threaded] 30+ 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
                   ` (13 subsequent siblings)
  15 siblings, 0 replies; 30+ 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	[flat|threaded] 30+ 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
                   ` (12 subsequent siblings)
  15 siblings, 0 replies; 30+ 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	[flat|threaded] 30+ 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
                   ` (11 subsequent siblings)
  15 siblings, 0 replies; 30+ 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	[flat|threaded] 30+ 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
                   ` (10 subsequent siblings)
  15 siblings, 1 reply; 30+ 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	[flat|threaded] 30+ 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
                   ` (9 subsequent siblings)
  15 siblings, 0 replies; 30+ 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	[flat|threaded] 30+ 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: report error on failure to fopen() Nguyễn Thái Ngọc Duy
                   ` (8 subsequent siblings)
  15 siblings, 1 reply; 30+ 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	[flat|threaded] 30+ 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
                   ` (7 subsequent siblings)
  15 siblings, 0 replies; 30+ 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	[flat|threaded] 30+ 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: 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 10/15] sequencer.c: report error on failure to fopen() Nguyễn Thái Ngọc Duy
                   ` (6 subsequent siblings)
  15 siblings, 0 replies; 30+ 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	[flat|threaded] 30+ 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: report error on failure to fopen() Nguyễn Thái Ngọc Duy
                   ` (5 subsequent siblings)
  15 siblings, 0 replies; 30+ 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	[flat|threaded] 30+ 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: report error on failure to fopen() Nguyễn Thái Ngọc Duy
                   ` (4 subsequent siblings)
  15 siblings, 0 replies; 30+ 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	[flat|threaded] 30+ 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: 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 13/15] xdiff-interface.c: report errno on failure to stat() or fopen() Nguyễn Thái Ngọc Duy
                   ` (3 subsequent siblings)
  15 siblings, 0 replies; 30+ 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	[flat|threaded] 30+ 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: 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 14/15] config.c: handle error on failing to fopen() Nguyễn Thái Ngọc Duy
                   ` (2 subsequent siblings)
  15 siblings, 0 replies; 30+ 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	[flat|threaded] 30+ 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
  2017-04-21  1:47 ` Junio C Hamano
  15 siblings, 0 replies; 30+ 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	[flat|threaded] 30+ 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 ` Junio C Hamano
  15 siblings, 0 replies; 30+ 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	[flat|threaded] 30+ 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
  15 siblings, 1 reply; 30+ 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|threaded] 30+ messages in thread

* Re: [PATCH 00/15] Handle fopen() errors
  2017-04-21  1:47 ` Junio C Hamano
@ 2017-04-21  3:41   ` Junio C Hamano
  2017-04-21  6:29     ` Jeff King
  0 siblings, 1 reply; 30+ 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	[flat|threaded] 30+ 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; 30+ 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|threaded] 30+ 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; 30+ 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|threaded] 30+ 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; 30+ 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|threaded] 30+ 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; 30+ 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|threaded] 30+ 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; 30+ 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|threaded] 30+ 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; 30+ 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|threaded] 30+ 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; 30+ 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|threaded] 30+ 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; 30+ 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|threaded] 30+ 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; 30+ 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|threaded] 30+ 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; 30+ 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|threaded] 30+ 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; 30+ 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|threaded] 30+ 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; 30+ 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|threaded] 30+ messages in thread

end of thread, back to index

Thread overview: 30+ 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: report error on failure to fopen() 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: report error on failure to fopen() Nguyễn Thái Ngọc Duy
2017-04-20 11:26 ` [PATCH 12/15] wt-status.c: report error on failure to fopen() 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 ` 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

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox