* [PATCH v3 01/13] git_fopen: fix a sparse 'not declared' warning
2017-05-26 3:34 ` [PATCH v3 00/13] reporting unexpected errors after (f)open Junio C Hamano
@ 2017-05-26 3:34 ` Junio C Hamano
2017-05-26 3:34 ` [PATCH v3 02/13] use xfopen() in more places Junio C Hamano
` (11 subsequent siblings)
12 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2017-05-26 3:34 UTC (permalink / raw)
To: git; +Cc: Ramsay Jones
From: Ramsay Jones <ramsay@ramsayjones.plus.com>
If git is built with the FREAD_READS_DIRECTORIES build variable set, this
would cause sparse to issue a 'not declared, should it be static?' warning
on Linux. This is a result of the method employed by 'compat/fopen.c' to
suppress the (possible) redefinition of the (system) fopen macro, which
also removes the extern declaration of the git_fopen function.
In order to suppress the warning, introduce a new macro to suppress the
definition (or possibly the re-definition) of the fopen symbol as a macro
override. This new macro (SUPPRESS_FOPEN_REDEFINITION) is only defined in
'compat/fopen.c', just prior to the inclusion of the 'git-compat-util.h'
header file.
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
compat/fopen.c | 4 ++--
git-compat-util.h | 10 ++++++----
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/compat/fopen.c b/compat/fopen.c
index b5ca142fed..107b3e8182 100644
--- a/compat/fopen.c
+++ b/compat/fopen.c
@@ -1,14 +1,14 @@
/*
* The order of the following two lines is important.
*
- * FREAD_READS_DIRECTORIES is undefined before including git-compat-util.h
+ * SUPPRESS_FOPEN_REDEFINITION is defined before including git-compat-util.h
* to avoid the redefinition of fopen within git-compat-util.h. This is
* necessary since fopen is a macro on some platforms which may be set
* based on compiler options. For example, on AIX fopen is set to fopen64
* when _LARGE_FILES is defined. The previous technique of merely undefining
* fopen after including git-compat-util.h is inadequate in this case.
*/
-#undef FREAD_READS_DIRECTORIES
+#define SUPPRESS_FOPEN_REDEFINITION
#include "../git-compat-util.h"
FILE *git_fopen(const char *path, const char *mode)
diff --git a/git-compat-util.h b/git-compat-util.h
index bd04564a69..6be55cf8b3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -689,10 +689,12 @@ char *gitstrdup(const char *s);
#endif
#ifdef FREAD_READS_DIRECTORIES
-#ifdef fopen
-#undef fopen
-#endif
-#define fopen(a,b) git_fopen(a,b)
+# if !defined(SUPPRESS_FOPEN_REDEFINITION)
+# ifdef fopen
+# undef fopen
+# endif
+# define fopen(a,b) git_fopen(a,b)
+# endif
extern FILE *git_fopen(const char*, const char*);
#endif
--
2.13.0-491-g71cfeddc25
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 02/13] use xfopen() in more places
2017-05-26 3:34 ` [PATCH v3 00/13] reporting unexpected errors after (f)open Junio C Hamano
2017-05-26 3:34 ` [PATCH v3 01/13] git_fopen: fix a sparse 'not declared' warning Junio C Hamano
@ 2017-05-26 3:34 ` Junio C Hamano
2017-05-26 3:35 ` [PATCH v3 03/13] clone: use xfopen() instead of fopen() Junio C Hamano
` (10 subsequent siblings)
12 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2017-05-26 3:34 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
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>
Signed-off-by: Junio C Hamano <gitster@pobox.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.13.0-491-g71cfeddc25
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 03/13] clone: use xfopen() instead of fopen()
2017-05-26 3:34 ` [PATCH v3 00/13] reporting unexpected errors after (f)open Junio C Hamano
2017-05-26 3:34 ` [PATCH v3 01/13] git_fopen: fix a sparse 'not declared' warning Junio C Hamano
2017-05-26 3:34 ` [PATCH v3 02/13] use xfopen() in more places Junio C Hamano
@ 2017-05-26 3:35 ` Junio C Hamano
2017-05-26 3:35 ` [PATCH v3 04/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD Junio C Hamano
` (9 subsequent siblings)
12 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2017-05-26 3:35 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
copy_alternates() called fopen() without handling errors. By switching
to xfopen(), this bug is fixed.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.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.13.0-491-g71cfeddc25
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 04/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD
2017-05-26 3:34 ` [PATCH v3 00/13] reporting unexpected errors after (f)open Junio C Hamano
` (2 preceding siblings ...)
2017-05-26 3:35 ` [PATCH v3 03/13] clone: use xfopen() instead of fopen() Junio C Hamano
@ 2017-05-26 3:35 ` Junio C Hamano
2017-05-26 3:35 ` [PATCH v3 05/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too Junio C Hamano
` (8 subsequent siblings)
12 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2017-05-26 3:35 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
config.mak.uname | 3 +++
t/t1308-config-set.sh | 8 ++++++++
2 files changed, 11 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
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index ff50960cca..72d5f1f931 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -183,6 +183,14 @@ 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 "^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.13.0-491-g71cfeddc25
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 05/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too
2017-05-26 3:34 ` [PATCH v3 00/13] reporting unexpected errors after (f)open Junio C Hamano
` (3 preceding siblings ...)
2017-05-26 3:35 ` [PATCH v3 04/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD Junio C Hamano
@ 2017-05-26 3:35 ` Junio C Hamano
2017-05-30 18:51 ` Stefan Beller
2017-05-26 3:35 ` [PATCH v3 06/13] wrapper.c: add and use warn_on_fopen_errors() Junio C Hamano
` (7 subsequent siblings)
12 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2017-05-26 3:35 UTC (permalink / raw)
To: git
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-491-g71cfeddc25
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v3 05/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too
2017-05-26 3:35 ` [PATCH v3 05/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too Junio C Hamano
@ 2017-05-30 18:51 ` Stefan Beller
2017-05-30 23:14 ` Junio C Hamano
0 siblings, 1 reply; 41+ messages in thread
From: Stefan Beller @ 2017-05-30 18:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
Do we have any reasons for that, or pointers on the mailing list, that this
is a good idea or needed? Does it fix a bug or enable a new feature on Darwin?
> 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-491-g71cfeddc25
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 05/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too
2017-05-30 18:51 ` Stefan Beller
@ 2017-05-30 23:14 ` Junio C Hamano
2017-05-30 23:32 ` Stefan Beller
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2017-05-30 23:14 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
Stefan Beller <sbeller@google.com> writes:
> On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Do we have any reasons for that, or pointers on the mailing list, that this
> is a good idea or needed? Does it fix a bug or enable a new feature on Darwin?
BSD lets you open() a directory for reading and hence we require
this workaround on it; I think somebody in an earlier round
suspected that that Darwin would have inherited the same from its
BSD lineage, and I don't see a reason to contradict with that
suspicion, either.
>
>> 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-491-g71cfeddc25
>>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 05/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too
2017-05-30 23:14 ` Junio C Hamano
@ 2017-05-30 23:32 ` Stefan Beller
0 siblings, 0 replies; 41+ messages in thread
From: Stefan Beller @ 2017-05-30 23:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
On Tue, May 30, 2017 at 4:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Do we have any reasons for that, or pointers on the mailing list, that this
>> is a good idea or needed? Does it fix a bug or enable a new feature on Darwin?
>
> BSD lets you open() a directory for reading and hence we require
> this workaround on it; I think somebody in an earlier round
> suspected that that Darwin would have inherited the same from its
> BSD lineage, and I don't see a reason to contradict with that
> suspicion, either.
>
Sorry about not being clear:
I - just like some future reader - did not have the context of the previous
round, so please put this information into the commit message.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v3 06/13] wrapper.c: add and use warn_on_fopen_errors()
2017-05-26 3:34 ` [PATCH v3 00/13] reporting unexpected errors after (f)open Junio C Hamano
` (4 preceding siblings ...)
2017-05-26 3:35 ` [PATCH v3 05/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too Junio C Hamano
@ 2017-05-26 3:35 ` Junio C Hamano
2017-05-26 3:35 ` [PATCH v3 07/13] wrapper.c: add and use fopen_or_warn() Junio C Hamano
` (6 subsequent siblings)
12 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2017-05-26 3:35 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
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.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
config.c | 3 +++
dir.c | 6 +++---
git-compat-util.h | 2 ++
t/t1308-config-set.sh | 3 ++-
wrapper.c | 10 ++++++++++
5 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/config.c b/config.c
index b4a3205da3..2894fbb6d0 100644
--- a/config.c
+++ b/config.c
@@ -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/dir.c b/dir.c
index f451bfa48c..be616e884e 100644
--- a/dir.c
+++ b/dir.c
@@ -745,9 +745,9 @@ 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);
- if (0 <= fd)
+ if (fd < 0)
+ warn_on_fopen_errors(fname);
+ else
close(fd);
if (!check_index ||
(buf = read_skip_worktree_file_from_index(fname, &size, sha1_stat)) == NULL)
diff --git a/git-compat-util.h b/git-compat-util.h
index 6be55cf8b3..eb5c18c7fd 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1101,6 +1101,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/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 72d5f1f931..df92fdcd40 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -195,7 +195,8 @@ 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 "^Error" output >actual &&
test_cmp expect actual
'
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.13.0-491-g71cfeddc25
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 07/13] wrapper.c: add and use fopen_or_warn()
2017-05-26 3:34 ` [PATCH v3 00/13] reporting unexpected errors after (f)open Junio C Hamano
` (5 preceding siblings ...)
2017-05-26 3:35 ` [PATCH v3 06/13] wrapper.c: add and use warn_on_fopen_errors() Junio C Hamano
@ 2017-05-26 3:35 ` Junio C Hamano
2017-05-26 3:35 ` [PATCH v3 08/13] wrapper.c: make warn_on_inaccessible() static Junio C Hamano
` (5 subsequent siblings)
12 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2017-05-26 3:35 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
attr.c | 7 ++-----
bisect.c | 2 +-
builtin/blame.c | 2 +-
commit.c | 2 +-
config.c | 2 +-
git-compat-util.h | 1 +
ident.c | 8 +++-----
remote.c | 4 ++--
rerere.c | 2 +-
sequencer.c | 8 ++++----
server-info.c | 2 +-
t/t1308-config-set.sh | 2 ++
t/t5512-ls-remote.sh | 13 ++++++++++---
wrapper.c | 11 +++++++++++
wt-status.c | 3 ++-
15 files changed, 43 insertions(+), 26 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;
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;
diff --git a/builtin/blame.c b/builtin/blame.c
index 07506a3e45..34445d7894 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2071,7 +2071,7 @@ 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;
diff --git a/commit.c b/commit.c
index 73c78c2b80..3eeda081f9 100644
--- a/commit.c
+++ b/commit.c
@@ -167,7 +167,7 @@ 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;
diff --git a/config.c b/config.c
index 2894fbb6d0..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);
diff --git a/git-compat-util.h b/git-compat-util.h
index eb5c18c7fd..f74b401810 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -802,6 +802,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/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");
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/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;
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))
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. */
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index df92fdcd40..e495a61616 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -187,6 +187,7 @@ 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
'
@@ -196,6 +197,7 @@ test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' '
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>output &&
+ grep "^warning:" output &&
grep "^Error" output >actual &&
test_cmp expect actual
'
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
'
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;
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.13.0-491-g71cfeddc25
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 08/13] wrapper.c: make warn_on_inaccessible() static
2017-05-26 3:34 ` [PATCH v3 00/13] reporting unexpected errors after (f)open Junio C Hamano
` (6 preceding siblings ...)
2017-05-26 3:35 ` [PATCH v3 07/13] wrapper.c: add and use fopen_or_warn() Junio C Hamano
@ 2017-05-26 3:35 ` Junio C Hamano
2017-05-26 3:35 ` [PATCH v3 09/13] print errno when reporting a system call error Junio C Hamano
` (4 subsequent siblings)
12 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2017-05-26 3:35 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
After the last patch, this function is not used outside anymore. Keep it
static.
Noticed-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
git-compat-util.h | 2 --
wrapper.c | 10 +++++-----
2 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index f74b401810..87f47828a5 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1100,8 +1100,6 @@ int remove_or_warn(unsigned int mode, const char *path);
int access_or_warn(const char *path, int mode, unsigned flag);
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);
diff --git a/wrapper.c b/wrapper.c
index 6e513c904a..b117eb14a4 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -418,6 +418,11 @@ FILE *fopen_for_writing(const char *path)
return ret;
}
+static void warn_on_inaccessible(const char *path)
+{
+ warning_errno(_("unable to access '%s'"), path);
+}
+
int warn_on_fopen_errors(const char *path)
{
if (errno != ENOENT && errno != ENOTDIR) {
@@ -597,11 +602,6 @@ int remove_or_warn(unsigned int mode, const char *file)
return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
}
-void warn_on_inaccessible(const char *path)
-{
- warning_errno(_("unable to access '%s'"), path);
-}
-
static int access_error_is_ok(int err, unsigned flag)
{
return err == ENOENT || err == ENOTDIR ||
--
2.13.0-491-g71cfeddc25
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 09/13] print errno when reporting a system call error
2017-05-26 3:34 ` [PATCH v3 00/13] reporting unexpected errors after (f)open Junio C Hamano
` (7 preceding siblings ...)
2017-05-26 3:35 ` [PATCH v3 08/13] wrapper.c: make warn_on_inaccessible() static Junio C Hamano
@ 2017-05-26 3:35 ` Junio C Hamano
2017-05-26 3:35 ` [PATCH v3 10/13] rerere.c: move error_errno() closer to the source system call Junio C Hamano
` (3 subsequent siblings)
12 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2017-05-26 3:35 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/log.c | 3 ++-
rerere.c | 4 ++--
xdiff-interface.c | 4 ++--
3 files changed, 6 insertions(+), 5 deletions(-)
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;
diff --git a/rerere.c b/rerere.c
index 971bfedfb2..1351b0c3fb 100644
--- a/rerere.c
+++ b/rerere.c
@@ -484,13 +484,13 @@ 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) {
fclose(io.input);
- return error("Could not write %s", output);
+ return error_errno("Could not write %s", output);
}
}
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.13.0-491-g71cfeddc25
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 10/13] rerere.c: move error_errno() closer to the source system call
2017-05-26 3:34 ` [PATCH v3 00/13] reporting unexpected errors after (f)open Junio C Hamano
` (8 preceding siblings ...)
2017-05-26 3:35 ` [PATCH v3 09/13] print errno when reporting a system call error Junio C Hamano
@ 2017-05-26 3:35 ` Junio C Hamano
2017-05-30 18:58 ` Stefan Beller
2017-05-26 3:35 ` [PATCH v3 11/13] log: fix memory leak in open_next_file() Junio C Hamano
` (2 subsequent siblings)
12 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2017-05-26 3:35 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
We are supposed to report errno from fopen(). fclose() between fopen()
and the report function could either change errno or reset it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
rerere.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/rerere.c b/rerere.c
index 1351b0c3fb..c26c29f87a 100644
--- a/rerere.c
+++ b/rerere.c
@@ -489,8 +489,9 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output
if (output) {
io.io.output = fopen(output, "w");
if (!io.io.output) {
+ error_errno("Could not write %s", output);
fclose(io.input);
- return error_errno("Could not write %s", output);
+ return -1;
}
}
--
2.13.0-491-g71cfeddc25
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v3 10/13] rerere.c: move error_errno() closer to the source system call
2017-05-26 3:35 ` [PATCH v3 10/13] rerere.c: move error_errno() closer to the source system call Junio C Hamano
@ 2017-05-30 18:58 ` Stefan Beller
0 siblings, 0 replies; 41+ messages in thread
From: Stefan Beller @ 2017-05-30 18:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Nguyễn Thái Ngọc Duy
On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> We are supposed to report errno from fopen(). fclose() between fopen()
> and the report function could either change errno or reset it.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> rerere.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/rerere.c b/rerere.c
> index 1351b0c3fb..c26c29f87a 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -489,8 +489,9 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output
> if (output) {
> io.io.output = fopen(output, "w");
> if (!io.io.output) {
> + error_errno("Could not write %s", output);
> fclose(io.input);
> - return error_errno("Could not write %s", output);
> + return -1;
and here we assume fclose doesn't error out.
This is ok as we are sure it cannot be EBADF as we just opened that
fd and as we did not read from io.input yet, any errno from close(2), fflush(3)
are not expected?
close(2) may yield an EINTR, in which case we may want to
while (fclose(io.input) && errno != EINTR)
; /*do nothing*/
On the other hand this is already in the error path, so maybe we're ok
leaking an fd here in case of a signal?
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v3 11/13] log: fix memory leak in open_next_file()
2017-05-26 3:34 ` [PATCH v3 00/13] reporting unexpected errors after (f)open Junio C Hamano
` (9 preceding siblings ...)
2017-05-26 3:35 ` [PATCH v3 10/13] rerere.c: move error_errno() closer to the source system call Junio C Hamano
@ 2017-05-26 3:35 ` Junio C Hamano
2017-05-26 3:35 ` [PATCH v3 12/13] wrapper: factor out is_missing_file_error() Junio C Hamano
2017-05-26 3:35 ` [PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows Junio C Hamano
12 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2017-05-26 3:35 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Noticed-by: Jeff King <peff@peff.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.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.13.0-491-g71cfeddc25
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 12/13] wrapper: factor out is_missing_file_error()
2017-05-26 3:34 ` [PATCH v3 00/13] reporting unexpected errors after (f)open Junio C Hamano
` (10 preceding siblings ...)
2017-05-26 3:35 ` [PATCH v3 11/13] log: fix memory leak in open_next_file() Junio C Hamano
@ 2017-05-26 3:35 ` Junio C Hamano
2017-05-30 0:31 ` [PATCH 1/2] compat-util: is_missing_file_error() Junio C Hamano
2017-05-26 3:35 ` [PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows Junio C Hamano
12 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2017-05-26 3:35 UTC (permalink / raw)
To: git
Our code often opens a path to an optional file, to work on its
contents when we can successfully open it. We can ignore a failure
to open if such an optional file does not exist, but we do want to
report a failure in opening for other reasons (e.g. we got an I/O
error, or the file is there, but we lack the permission to open).
There is a logic to determine if an errno left by open/fopen
indicates such an ignorable error. Split it out into a helper
function. We may want to make it an extern in later steps, as many
hits from "git grep ENOENT" would instead want to be using the same
logic.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
wrapper.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/wrapper.c b/wrapper.c
index b117eb14a4..f1c87ec7ea 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -423,9 +423,23 @@ static void warn_on_inaccessible(const char *path)
warning_errno(_("unable to access '%s'"), path);
}
+/*
+ * Our code often opens a path to an optional file, to work on its
+ * contents when we can successfully open it. We can ignore a failure
+ * to open if such an optional file does not exist, but we do want to
+ * report a failure in opening for other reasons (e.g. we got an I/O
+ * error, or the file is there, but we lack the permission to open).
+ *
+ * Call this function after seeing an error from open() or fopen() to
+ * see if the errno indicates a missing file that we can safely ignore.
+ */
+static int is_missing_file_error(int errno_) {
+ return (errno_ == ENOENT || errno_ == ENOTDIR);
+}
+
int warn_on_fopen_errors(const char *path)
{
- if (errno != ENOENT && errno != ENOTDIR) {
+ if (!is_missing_file_error(errno)) {
warn_on_inaccessible(path);
return -1;
}
--
2.13.0-491-g71cfeddc25
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 1/2] compat-util: is_missing_file_error()
2017-05-26 3:35 ` [PATCH v3 12/13] wrapper: factor out is_missing_file_error() Junio C Hamano
@ 2017-05-30 0:31 ` Junio C Hamano
2017-05-30 0:32 ` [PATCH 2/2] treewide: use is_missing_file_error() where ENOENT and ENOTDIR are checked Junio C Hamano
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2017-05-30 0:31 UTC (permalink / raw)
To: git
Our code often opens a path to an optional file, to work on its
contents when we can successfully open it. We can ignore a failure
to open if such an optional file does not exist, but we do want to
report a failure in opening for other reasons (e.g. we got an I/O
error, or the file is there, but we lack the permission to open).
The exact errors we need to ignore are ENOENT (obviously) and
ENOTDIR (less obvious). Instead of repeating comparison of errno
with these two constants, introduce a helper function to do so.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Now this is independent of nd/fopen-errors topic. We could tweak
nd/fopen-errors topic to use this later, after both of these two
topics have graduated.
git-compat-util.h | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 8a4a3f85e7..8a3c680626 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1115,6 +1115,21 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
#define getc_unlocked(fh) getc(fh)
#endif
+/*
+ * Our code often opens a path to an optional file, to work on its
+ * contents when we can successfully open it. We can ignore a failure
+ * to open if such an optional file does not exist, but we do want to
+ * report a failure in opening for other reasons (e.g. we got an I/O
+ * error, or the file is there, but we lack the permission to open).
+ *
+ * Call this function after seeing an error from open() or fopen() to
+ * see if the errno indicates a missing file that we can safely ignore.
+ */
+static inline int is_missing_file_error(int errno_)
+{
+ return (errno_ == ENOENT || errno_ == ENOTDIR);
+}
+
extern int cmd_main(int, const char **);
#endif
--
2.13.0-496-ga689fffbe2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 2/2] treewide: use is_missing_file_error() where ENOENT and ENOTDIR are checked
2017-05-30 0:31 ` [PATCH 1/2] compat-util: is_missing_file_error() Junio C Hamano
@ 2017-05-30 0:32 ` Junio C Hamano
0 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2017-05-30 0:32 UTC (permalink / raw)
To: git
Using the is_missing_file_error() helper introduced in the previous
step, update all hits from
$ git grep -e ENOENT --and -e ENOTDIR
There are codepaths that only check ENOENT, and it is possible that
some of them should be checking both. Updating them is kept out of
this step deliberately, as we do not want to change behaviour in this
step.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
apply.c | 2 +-
builtin/rm.c | 2 +-
builtin/update-index.c | 2 +-
diff-lib.c | 2 +-
dir.c | 2 +-
setup.c | 2 +-
sha1_name.c | 4 ++--
wrapper.c | 4 ++--
8 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/apply.c b/apply.c
index 0e2caeab9c..59bb3497de 100644
--- a/apply.c
+++ b/apply.c
@@ -3741,7 +3741,7 @@ static int check_to_create(struct apply_state *state,
return 0;
return EXISTS_IN_WORKTREE;
- } else if ((errno != ENOENT) && (errno != ENOTDIR)) {
+ } else if (!is_missing_file_error(errno)) {
return error_errno("%s", new_name);
}
return 0;
diff --git a/builtin/rm.c b/builtin/rm.c
index fb79dcab18..30c4332c68 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -129,7 +129,7 @@ static int check_local_mod(struct object_id *head, int index_only)
ce = active_cache[pos];
if (lstat(ce->name, &st) < 0) {
- if (errno != ENOENT && errno != ENOTDIR)
+ if (!is_missing_file_error(errno))
warning_errno(_("failed to stat '%s'"), ce->name);
/* It already vanished from the working tree */
continue;
diff --git a/builtin/update-index.c b/builtin/update-index.c
index d530e89368..4e9402984a 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -253,7 +253,7 @@ static int remove_one_path(const char *path)
*/
static int process_lstat_error(const char *path, int err)
{
- if (err == ENOENT || err == ENOTDIR)
+ if (is_missing_file_error(err))
return remove_one_path(path);
return error("lstat(\"%s\"): %s", path, strerror(err));
}
diff --git a/diff-lib.c b/diff-lib.c
index 52447466b5..88fc71e89e 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -29,7 +29,7 @@
static int check_removed(const struct cache_entry *ce, struct stat *st)
{
if (lstat(ce->name, st) < 0) {
- if (errno != ENOENT && errno != ENOTDIR)
+ if (!is_missing_file_error(errno))
return -1;
return 1;
}
diff --git a/dir.c b/dir.c
index aeeb5ce104..98efe9d1c5 100644
--- a/dir.c
+++ b/dir.c
@@ -2235,7 +2235,7 @@ int remove_path(const char *name)
{
char *slash;
- if (unlink(name) && errno != ENOENT && errno != ENOTDIR)
+ if (unlink(name) && !is_missing_file_error(errno))
return -1;
slash = strrchr(name, '/');
diff --git a/setup.c b/setup.c
index 8f64fbdfb2..bb6a2c1beb 100644
--- a/setup.c
+++ b/setup.c
@@ -147,7 +147,7 @@ int check_filename(const char *prefix, const char *arg)
name = arg;
if (!lstat(name, &st))
return 1; /* file exists */
- if (errno == ENOENT || errno == ENOTDIR)
+ if (is_missing_file_error(errno))
return 0; /* file does not exist */
die_errno("failed to stat '%s'", arg);
}
diff --git a/sha1_name.c b/sha1_name.c
index 26ceec1d79..af7500037d 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1406,7 +1406,7 @@ static void diagnose_invalid_sha1_path(const char *prefix,
if (file_exists(filename))
die("Path '%s' exists on disk, but not in '%.*s'.",
filename, object_name_len, object_name);
- if (errno == ENOENT || errno == ENOTDIR) {
+ if (is_missing_file_error(errno)) {
char *fullname = xstrfmt("%s%s", prefix, filename);
if (!get_tree_entry(tree_sha1, fullname,
@@ -1471,7 +1471,7 @@ static void diagnose_invalid_index_path(int stage,
if (file_exists(filename))
die("Path '%s' exists on disk, but not in the index.", filename);
- if (errno == ENOENT || errno == ENOTDIR)
+ if (is_missing_file_error(errno))
die("Path '%s' does not exist (neither on disk nor in the index).",
filename);
diff --git a/wrapper.c b/wrapper.c
index 0542fc7582..2fbbd81359 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -583,8 +583,8 @@ void warn_on_inaccessible(const char *path)
static int access_error_is_ok(int err, unsigned flag)
{
- return err == ENOENT || err == ENOTDIR ||
- ((flag & ACCESS_EACCES_OK) && err == EACCES);
+ return (is_missing_file_error(err) ||
+ ((flag & ACCESS_EACCES_OK) && err == EACCES));
}
int access_or_warn(const char *path, int mode, unsigned flag)
--
2.13.0-496-ga689fffbe2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows
2017-05-26 3:34 ` [PATCH v3 00/13] reporting unexpected errors after (f)open Junio C Hamano
` (11 preceding siblings ...)
2017-05-26 3:35 ` [PATCH v3 12/13] wrapper: factor out is_missing_file_error() Junio C Hamano
@ 2017-05-26 3:35 ` Junio C Hamano
2017-05-29 20:25 ` [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames Johannes Sixt
2017-05-30 19:13 ` [PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows Stefan Beller
12 siblings, 2 replies; 41+ messages in thread
From: Junio C Hamano @ 2017-05-26 3:35 UTC (permalink / raw)
To: git
When asked to open/fopen a path, e.g. "a/b:/c", which does not exist
on the filesystem, Windows (correctly) fails to open it but sets
EINVAL to errno because the pathname has characters that cannot be
stored in its filesystem.
As this is an expected failure, teach is_missing_file_error() helper
about this case.
This is RFC, as there may be a case where we get EINVAL from
open/fopen for reasons other than "the filesystem does not like this
pathname" that may be worth reporting to the user, and this change
is sweeping such an error under the rug.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
wrapper.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/wrapper.c b/wrapper.c
index f1c87ec7ea..74aa3b7803 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -434,6 +434,10 @@ static void warn_on_inaccessible(const char *path)
* see if the errno indicates a missing file that we can safely ignore.
*/
static int is_missing_file_error(int errno_) {
+#ifdef GIT_WINDOWS_NATIVE
+ if (errno_ == EINVAL)
+ return 1;
+#endif
return (errno_ == ENOENT || errno_ == ENOTDIR);
}
--
2.13.0-491-g71cfeddc25
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames
2017-05-26 3:35 ` [PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows Junio C Hamano
@ 2017-05-29 20:25 ` Johannes Sixt
2017-05-29 20:27 ` [PATCH 2/2] mingw_fopen: report ENOENT for invalid file names Johannes Sixt
` (2 more replies)
2017-05-30 19:13 ` [PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows Stefan Beller
1 sibling, 3 replies; 41+ messages in thread
From: Johannes Sixt @ 2017-05-29 20:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin
Am 26.05.2017 um 05:35 schrieb Junio C Hamano:
> When asked to open/fopen a path, e.g. "a/b:/c", which does not exist
> on the filesystem, Windows (correctly) fails to open it but sets
> EINVAL to errno because the pathname has characters that cannot be
> stored in its filesystem.
>
> As this is an expected failure, teach is_missing_file_error() helper
> about this case.
>
> This is RFC, as there may be a case where we get EINVAL from
> open/fopen for reasons other than "the filesystem does not like this
> pathname" that may be worth reporting to the user, and this change
> is sweeping such an error under the rug.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> wrapper.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/wrapper.c b/wrapper.c
> index f1c87ec7ea..74aa3b7803 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -434,6 +434,10 @@ static void warn_on_inaccessible(const char *path)
> * see if the errno indicates a missing file that we can safely ignore.
> */
> static int is_missing_file_error(int errno_) {
> +#ifdef GIT_WINDOWS_NATIVE
> + if (errno_ == EINVAL)
> + return 1;
> +#endif
> return (errno_ == ENOENT || errno_ == ENOTDIR);
> }
I would prefer to catch the case in the compatibility layer. Here is
a two patch series that would replace your 12/13 and 13/13.
---- 8< ----
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: mingw: verify that paths are not mistaken for remote nicknames
This added test case simply verifies that users will not be bothered
with bogus complaints à la
warning: unable to access '.git/remotes/D:\repo': Invalid argument
when fetching from a Windows path (in this case, D:\repo).
[j6t: mark the new test as test_expect_failure]
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
t/t5580-clone-push-unc.sh | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index b195f71ea9..fd719a209e 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -1,13 +1,19 @@
#!/bin/sh
-test_description='various UNC path tests (Windows-only)'
+test_description='various Windows-only path tests'
. ./test-lib.sh
if ! test_have_prereq MINGW; then
- skip_all='skipping UNC path tests, requires Windows'
+ skip_all='skipping Windows-only path tests'
test_done
fi
+test_expect_failure 'remote nick cannot contain backslashes' '
+ BACKSLASHED="$(pwd | tr / \\\\)" &&
+ git ls-remote "$BACKSLASHED" >out 2>err &&
+ ! grep "unable to access" err
+'
+
UNCPATH="$(pwd)"
case "$UNCPATH" in
[A-Z]:*)
--
2.13.0.55.g17b7d13330
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 2/2] mingw_fopen: report ENOENT for invalid file names
2017-05-29 20:25 ` [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames Johannes Sixt
@ 2017-05-29 20:27 ` Johannes Sixt
2017-05-29 20:40 ` [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames Ævar Arnfjörð Bjarmason
2017-05-30 0:29 ` Junio C Hamano
2 siblings, 0 replies; 41+ messages in thread
From: Johannes Sixt @ 2017-05-29 20:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin
On Windows, certain characters are prohibited in file names, most
prominently the colon. When fopen() is called with such an invalid file
name, the underlying Windows API actually reports a particular error,
but since there is no suitable errno value, this error is translated
to EINVAL. Detect the case and report ENOENT instead.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
compat/mingw.c | 2 ++
t/t5580-clone-push-unc.sh | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 62109cc4e6..ce6fe8f46b 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -423,6 +423,8 @@ FILE *mingw_fopen (const char *filename, const char
*otype)
return NULL;
}
file = _wfopen(wfilename, wotype);
+ if (!file && GetLastError() == ERROR_INVALID_NAME)
+ errno = ENOENT;
if (file && hide && set_hidden_flag(wfilename, 1))
warning("could not mark '%s' as hidden.", filename);
return file;
diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index fd719a209e..93ce99ba3c 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -8,7 +8,7 @@ if ! test_have_prereq MINGW; then
test_done
fi
-test_expect_failure 'remote nick cannot contain backslashes' '
+test_expect_success 'remote nick cannot contain backslashes' '
BACKSLASHED="$(pwd | tr / \\\\)" &&
git ls-remote "$BACKSLASHED" >out 2>err &&
! grep "unable to access" err
--
2.13.0.55.g17b7d13330
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames
2017-05-29 20:25 ` [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames Johannes Sixt
2017-05-29 20:27 ` [PATCH 2/2] mingw_fopen: report ENOENT for invalid file names Johannes Sixt
@ 2017-05-29 20:40 ` Ævar Arnfjörð Bjarmason
2017-05-29 21:02 ` Johannes Sixt
2017-05-30 0:29 ` Junio C Hamano
2 siblings, 1 reply; 41+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-05-29 20:40 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Git Mailing List, Johannes Schindelin
On Mon, May 29, 2017 at 10:25 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 26.05.2017 um 05:35 schrieb Junio C Hamano:
>> When asked to open/fopen a path, e.g. "a/b:/c", which does not exist
>> on the filesystem, Windows (correctly) fails to open it but sets
>> EINVAL to errno because the pathname has characters that cannot be
>> stored in its filesystem.
>>
>> As this is an expected failure, teach is_missing_file_error() helper
>> about this case.
>>
>> This is RFC, as there may be a case where we get EINVAL from
>> open/fopen for reasons other than "the filesystem does not like this
>> pathname" that may be worth reporting to the user, and this change
>> is sweeping such an error under the rug.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>> wrapper.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/wrapper.c b/wrapper.c
>> index f1c87ec7ea..74aa3b7803 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -434,6 +434,10 @@ static void warn_on_inaccessible(const char *path)
>> * see if the errno indicates a missing file that we can safely ignore.
>> */
>> static int is_missing_file_error(int errno_) {
>> +#ifdef GIT_WINDOWS_NATIVE
>> + if (errno_ == EINVAL)
>> + return 1;
>> +#endif
>> return (errno_ == ENOENT || errno_ == ENOTDIR);
>> }
>
> I would prefer to catch the case in the compatibility layer. Here is
> a two patch series that would replace your 12/13 and 13/13.
>
> ---- 8< ----
> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Subject: mingw: verify that paths are not mistaken for remote nicknames
>
> This added test case simply verifies that users will not be bothered
> with bogus complaints à la
>
> warning: unable to access '.git/remotes/D:\repo': Invalid argument
>
> when fetching from a Windows path (in this case, D:\repo).
>
> [j6t: mark the new test as test_expect_failure]
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> t/t5580-clone-push-unc.sh | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
> index b195f71ea9..fd719a209e 100755
> --- a/t/t5580-clone-push-unc.sh
> +++ b/t/t5580-clone-push-unc.sh
> @@ -1,13 +1,19 @@
> #!/bin/sh
>
> -test_description='various UNC path tests (Windows-only)'
> +test_description='various Windows-only path tests'
> . ./test-lib.sh
>
> if ! test_have_prereq MINGW; then
> - skip_all='skipping UNC path tests, requires Windows'
> + skip_all='skipping Windows-only path tests'
> test_done
> fi
>
> +test_expect_failure 'remote nick cannot contain backslashes' '
> + BACKSLASHED="$(pwd | tr / \\\\)" &&
> + git ls-remote "$BACKSLASHED" >out 2>err &&
> + ! grep "unable to access" err
> +'
Doesn't this need test_i18ngrep?:
wrapper.c:581: warning_errno(_("unable to access '%s'"), path);
wrapper.c:602: die_errno(_("unable to access '%s'"), path);
Or is it one of these, which isn't translated:
http-push.c:1531: error("unable to access '%s': %s",
url, curl_errorstr);
remote-curl.c:319: die("unable to access '%s': %s",
url.buf, curl_errorstr);
> +
> UNCPATH="$(pwd)"
> case "$UNCPATH" in
> [A-Z]:*)
> --
> 2.13.0.55.g17b7d13330
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames
2017-05-29 20:40 ` [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames Ævar Arnfjörð Bjarmason
@ 2017-05-29 21:02 ` Johannes Sixt
2017-05-29 21:59 ` Ramsay Jones
` (2 more replies)
0 siblings, 3 replies; 41+ messages in thread
From: Johannes Sixt @ 2017-05-29 21:02 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, Johannes Schindelin
Cc: Junio C Hamano, Git Mailing List
Am 29.05.2017 um 22:40 schrieb Ævar Arnfjörð Bjarmason:
> On Mon, May 29, 2017 at 10:25 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
>> index b195f71ea9..fd719a209e 100755
>> --- a/t/t5580-clone-push-unc.sh
>> +++ b/t/t5580-clone-push-unc.sh
>> @@ -1,13 +1,19 @@
>> #!/bin/sh
>>
>> -test_description='various UNC path tests (Windows-only)'
>> +test_description='various Windows-only path tests'
>> . ./test-lib.sh
>>
>> if ! test_have_prereq MINGW; then
>> - skip_all='skipping UNC path tests, requires Windows'
>> + skip_all='skipping Windows-only path tests'
>> test_done
>> fi
>>
>> +test_expect_failure 'remote nick cannot contain backslashes' '
>> + BACKSLASHED="$(pwd | tr / \\\\)" &&
>> + git ls-remote "$BACKSLASHED" >out 2>err &&
>> + ! grep "unable to access" err
>> +'
>
> Doesn't this need test_i18ngrep?:
Good catch! It would be this one in warn_on_inaccessible:
> wrapper.c:581: warning_errno(_("unable to access '%s'"), path);
But actually, I'm more worried about the unholy mix of
one-test-first-then-skip_all-later that occurs in this test script (I do
not mean the skip_all that is visible in the context, there are others
later). I think there was some buzz recently that prove only understands
a summary line that reads "1..0", but here we would see "1..1". What to
do? Reorganize the test script? Dscho, any ideas?
-- Hannes
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames
2017-05-29 21:02 ` Johannes Sixt
@ 2017-05-29 21:59 ` Ramsay Jones
2017-05-30 0:03 ` Junio C Hamano
2017-05-29 23:53 ` Junio C Hamano
2017-05-30 4:46 ` Junio C Hamano
2 siblings, 1 reply; 41+ messages in thread
From: Ramsay Jones @ 2017-05-29 21:59 UTC (permalink / raw)
To: Johannes Sixt, Ævar Arnfjörð Bjarmason,
Johannes Schindelin
Cc: Junio C Hamano, Git Mailing List
On 29/05/17 22:02, Johannes Sixt wrote:
> Am 29.05.2017 um 22:40 schrieb Ævar Arnfjörð Bjarmason:
>> On Mon, May 29, 2017 at 10:25 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>>> diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
>>> index b195f71ea9..fd719a209e 100755
>>> --- a/t/t5580-clone-push-unc.sh
>>> +++ b/t/t5580-clone-push-unc.sh
>>> @@ -1,13 +1,19 @@
>>> #!/bin/sh
>>>
>>> -test_description='various UNC path tests (Windows-only)'
>>> +test_description='various Windows-only path tests'
>>> . ./test-lib.sh
>>>
>>> if ! test_have_prereq MINGW; then
>>> - skip_all='skipping UNC path tests, requires Windows'
>>> + skip_all='skipping Windows-only path tests'
>>> test_done
>>> fi
>>>
>>> +test_expect_failure 'remote nick cannot contain backslashes' '
>>> + BACKSLASHED="$(pwd | tr / \\\\)" &&
>>> + git ls-remote "$BACKSLASHED" >out 2>err &&
>>> + ! grep "unable to access" err
>>> +'
>>
>> Doesn't this need test_i18ngrep?:
>
> Good catch! It would be this one in warn_on_inaccessible:
>
>> wrapper.c:581: warning_errno(_("unable to access '%s'"), path);
>
> But actually, I'm more worried about the unholy mix of one-test-first-then-skip_all-later that occurs in this test script (I do not mean the skip_all that is visible in the context, there are others later). I think there was some buzz recently that prove only understands a summary line that reads "1..0", but here we would see "1..1". What to do? Reorganize the test script? Dscho, any ideas?
See commit c7018be509 ("test: allow skipping the remainder", 18-05-2017)
which is currently merged to the 'next' branch (merge 03b8a61e47 of the
'jc/skip-test-in-the-middle' branch).
(see also http://testanything.org)
If you look at http://testanything.org//tap-specification.html, it shows
that you are allowed to annotate a plan of '1..0' with a SKIP directive
to explain why no tests in a file were run. However, a plan with '1..n'
(for any n > 0) must not have any annotation. (Back in 2012, when I wrote
commit bf4b721932, I found much better documentation than the above!)
So, after commit c7018be509, you can now use the 'skip_all' facility
after having run some tests; it now converts that into an 'SKIP comment'
just before the test plan, effectively skipping the remainder of the
tests in the file. (since we are using a 'trailing plan', and have thus
not declared how many tests we had intended to run, we can output an
accurate plan).
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames
2017-05-29 21:59 ` Ramsay Jones
@ 2017-05-30 0:03 ` Junio C Hamano
2017-05-30 13:40 ` Ramsay Jones
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2017-05-30 0:03 UTC (permalink / raw)
To: Ramsay Jones
Cc: Johannes Sixt, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Git Mailing List
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> See commit c7018be509 ("test: allow skipping the remainder", 18-05-2017)
> which is currently merged to the 'next' branch (merge 03b8a61e47 of the
> 'jc/skip-test-in-the-middle' branch).
>
> (see also http://testanything.org)
>
> If you look at http://testanything.org//tap-specification.html, it shows
> that you are allowed to annotate a plan of '1..0' with a SKIP directive
> to explain why no tests in a file were run. However, a plan with '1..n'
> (for any n > 0) must not have any annotation. (Back in 2012, when I wrote
> commit bf4b721932, I found much better documentation than the above!)
>
> So, after commit c7018be509, you can now use the 'skip_all' facility
> after having run some tests; it now converts that into an 'SKIP comment'
> just before the test plan, effectively skipping the remainder of the
> tests in the file. (since we are using a 'trailing plan', and have thus
> not declared how many tests we had intended to run, we can output an
> accurate plan).
Yes, but I consider that c7018be509 is an ugly workaround, not a
part of a properly designed framework. Unless it is absolutely
necessary to run some tests before we may conditionally want to say
"skip_all/test_done", we should strive to add tests _after_ these
conditional skil_all/test_done is done.
In this case, I do not see there is a strong reason why the new test
must come before the "setup" test. Sure, it does not use UNCPATH so
the new test may be able to run even when the current path cannot be
spelled as UNC, but that is a natural fallout of (ab)using the test
script that was meant for UNC testing for something else, so I think
a proper way out would be either (1) to use a separate script, if
the new test wants to run whether UNC path can be determined, or (2)
just accept the fact that the new test will only be run when UNC
paths are tested. Relying on the hack c7018be509 did is much less
appealing.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames
2017-05-30 0:03 ` Junio C Hamano
@ 2017-05-30 13:40 ` Ramsay Jones
0 siblings, 0 replies; 41+ messages in thread
From: Ramsay Jones @ 2017-05-30 13:40 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Sixt, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, Git Mailing List
On 30/05/17 01:03, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
>> See commit c7018be509 ("test: allow skipping the remainder", 18-05-2017)
>> which is currently merged to the 'next' branch (merge 03b8a61e47 of the
>> 'jc/skip-test-in-the-middle' branch).
>>
>> (see also http://testanything.org)
>>
>> If you look at http://testanything.org//tap-specification.html, it shows
>> that you are allowed to annotate a plan of '1..0' with a SKIP directive
>> to explain why no tests in a file were run. However, a plan with '1..n'
>> (for any n > 0) must not have any annotation. (Back in 2012, when I wrote
>> commit bf4b721932, I found much better documentation than the above!)
>>
>> So, after commit c7018be509, you can now use the 'skip_all' facility
>> after having run some tests; it now converts that into an 'SKIP comment'
>> just before the test plan, effectively skipping the remainder of the
>> tests in the file. (since we are using a 'trailing plan', and have thus
>> not declared how many tests we had intended to run, we can output an
>> accurate plan).
>
> Yes, but I consider that c7018be509 is an ugly workaround, not a
> part of a properly designed framework. Unless it is absolutely
> necessary to run some tests before we may conditionally want to say
> "skip_all/test_done", we should strive to add tests _after_ these
> conditional skil_all/test_done is done.
yes, I don't disagree with that (which is why I said that I would
have split t5545 into two files). ;-)
> In this case, I do not see there is a strong reason why the new test
> must come before the "setup" test. Sure, it does not use UNCPATH so
> the new test may be able to run even when the current path cannot be
> spelled as UNC, but that is a natural fallout of (ab)using the test
> script that was meant for UNC testing for something else, so I think
> a proper way out would be either (1) to use a separate script, if
> the new test wants to run whether UNC path can be determined,
Yes, I had intended to suggest this (or an existing script, protected
by the MINGW prerequisite), but forgot!
> or (2)
> just accept the fact that the new test will only be run when UNC
> paths are tested.
I prefer (1).
> Relying on the hack c7018be509 did is much less
> appealing.
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames
2017-05-29 21:02 ` Johannes Sixt
2017-05-29 21:59 ` Ramsay Jones
@ 2017-05-29 23:53 ` Junio C Hamano
2017-05-30 4:46 ` Junio C Hamano
2 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2017-05-29 23:53 UTC (permalink / raw)
To: Johannes Sixt
Cc: Ævar Arnfjörð Bjarmason, Johannes Schindelin,
Git Mailing List
Johannes Sixt <j6t@kdbg.org> writes:
> Am 29.05.2017 um 22:40 schrieb Ævar Arnfjörð Bjarmason:
>> On Mon, May 29, 2017 at 10:25 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>>> diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
>>> index b195f71ea9..fd719a209e 100755
>>> --- a/t/t5580-clone-push-unc.sh
>>> +++ b/t/t5580-clone-push-unc.sh
>>> @@ -1,13 +1,19 @@
>>> #!/bin/sh
>>>
>>> -test_description='various UNC path tests (Windows-only)'
>>> +test_description='various Windows-only path tests'
>>> . ./test-lib.sh
>>>
>>> if ! test_have_prereq MINGW; then
>>> - skip_all='skipping UNC path tests, requires Windows'
>>> + skip_all='skipping Windows-only path tests'
>>> test_done
>>> fi
>>>
>>> +test_expect_failure 'remote nick cannot contain backslashes' '
>>> + BACKSLASHED="$(pwd | tr / \\\\)" &&
>>> + git ls-remote "$BACKSLASHED" >out 2>err &&
>>> + ! grep "unable to access" err
>>> +'
>>
>> Doesn't this need test_i18ngrep?:
>
> Good catch! It would be this one in warn_on_inaccessible:
>
>> wrapper.c:581: warning_errno(_("unable to access '%s'"), path);
>
> But actually, I'm more worried about the unholy mix of
> one-test-first-then-skip_all-later that occurs in this test script (I
> do not mean the skip_all that is visible in the context, there are
> others later). I think there was some buzz recently that prove only
> understands a summary line that reads "1..0", but here we would see
> "1..1". What to do? Reorganize the test script? Dscho, any ideas?
Put this new test after the other skip_all/test_done and you'd be
fine, I think. It should come after the "setup" test anyway, no?
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames
2017-05-29 21:02 ` Johannes Sixt
2017-05-29 21:59 ` Ramsay Jones
2017-05-29 23:53 ` Junio C Hamano
@ 2017-05-30 4:46 ` Junio C Hamano
2017-05-30 20:35 ` Johannes Sixt
2 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2017-05-30 4:46 UTC (permalink / raw)
To: Johannes Sixt
Cc: Ævar Arnfjörð Bjarmason, Johannes Schindelin,
Git Mailing List
Johannes Sixt <j6t@kdbg.org> writes:
>> Doesn't this need test_i18ngrep?:
>
> Good catch! It would be this one in warn_on_inaccessible:
>
>> wrapper.c:581: warning_errno(_("unable to access '%s'"), path);
>
> But actually, I'm more worried about the unholy mix of
> one-test-first-then-skip_all-later that occurs in this test script (I
> do not mean the skip_all that is visible in the context, there are
> others later). I think there was some buzz recently that prove only
> understands a summary line that reads "1..0", but here we would see
> "1..1". What to do? Reorganize the test script? Dscho, any ideas?
For now I've queued this on top of 1/2, so that suggestions are not
lost, and then tweaked 2/2 (as context for the patch to the test
changes).
Either an ack or a reroll is appreciated (I do not think we'd
terribly mind if this test were added to another script, or if this
test were skipped when UNC path cannot be determined even though it
does not need that prereq. Also UNC_PATH can become prereq that is
tested by individual test in this script and the new test can be
added without requiring that prereq).
Thanks.
t/t5580-clone-push-unc.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index fd719a209e..944730cddc 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -8,12 +8,6 @@ if ! test_have_prereq MINGW; then
test_done
fi
-test_expect_failure 'remote nick cannot contain backslashes' '
- BACKSLASHED="$(pwd | tr / \\\\)" &&
- git ls-remote "$BACKSLASHED" >out 2>err &&
- ! grep "unable to access" err
-'
-
UNCPATH="$(pwd)"
case "$UNCPATH" in
[A-Z]:*)
@@ -51,4 +45,10 @@ test_expect_success push '
test "$rev" = "$(git rev-parse --verify refs/heads/to-push)"
'
+test_expect_failure 'remote nick cannot contain backslashes' '
+ BACKSLASHED="$(pwd | tr / \\\\)" &&
+ git ls-remote "$BACKSLASHED" >out 2>err &&
+ test_i18ngrep ! "unable to access" err
+'
+
test_done
--
2.13.0-493-g9105ebc082
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames
2017-05-30 4:46 ` Junio C Hamano
@ 2017-05-30 20:35 ` Johannes Sixt
0 siblings, 0 replies; 41+ messages in thread
From: Johannes Sixt @ 2017-05-30 20:35 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, Johannes Schindelin,
Git Mailing List
Am 30.05.2017 um 06:46 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>>> Doesn't this need test_i18ngrep?:
>>
>> Good catch! It would be this one in warn_on_inaccessible:
>>
>>> wrapper.c:581: warning_errno(_("unable to access '%s'"), path);
>>
>> But actually, I'm more worried about the unholy mix of
>> one-test-first-then-skip_all-later that occurs in this test script (I
>> do not mean the skip_all that is visible in the context, there are
>> others later). I think there was some buzz recently that prove only
>> understands a summary line that reads "1..0", but here we would see
>> "1..1". What to do? Reorganize the test script? Dscho, any ideas?
>
> For now I've queued this on top of 1/2, so that suggestions are not
> lost, and then tweaked 2/2 (as context for the patch to the test
> changes).
>
> Either an ack or a reroll is appreciated (I do not think we'd
> terribly mind if this test were added to another script, or if this
> test were skipped when UNC path cannot be determined even though it
> does not need that prereq. Also UNC_PATH can become prereq that is
> tested by individual test in this script and the new test can be
> added without requiring that prereq).
>
> Thanks.
>
> t/t5580-clone-push-unc.sh | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
> index fd719a209e..944730cddc 100755
> --- a/t/t5580-clone-push-unc.sh
> +++ b/t/t5580-clone-push-unc.sh
> @@ -8,12 +8,6 @@ if ! test_have_prereq MINGW; then
> test_done
> fi
>
> -test_expect_failure 'remote nick cannot contain backslashes' '
> - BACKSLASHED="$(pwd | tr / \\\\)" &&
> - git ls-remote "$BACKSLASHED" >out 2>err &&
> - ! grep "unable to access" err
> -'
> -
> UNCPATH="$(pwd)"
> case "$UNCPATH" in
> [A-Z]:*)
> @@ -51,4 +45,10 @@ test_expect_success push '
> test "$rev" = "$(git rev-parse --verify refs/heads/to-push)"
> '
>
> +test_expect_failure 'remote nick cannot contain backslashes' '
> + BACKSLASHED="$(pwd | tr / \\\\)" &&
> + git ls-remote "$BACKSLASHED" >out 2>err &&
> + test_i18ngrep ! "unable to access" err
> +'
> +
> test_done
>
ACK!
-- Hannes
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames
2017-05-29 20:25 ` [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames Johannes Sixt
2017-05-29 20:27 ` [PATCH 2/2] mingw_fopen: report ENOENT for invalid file names Johannes Sixt
2017-05-29 20:40 ` [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames Ævar Arnfjörð Bjarmason
@ 2017-05-30 0:29 ` Junio C Hamano
2 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2017-05-30 0:29 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, Johannes Schindelin
Johannes Sixt <j6t@kdbg.org> writes:
> I would prefer to catch the case in the compatibility layer. Here is
> a two patch series that would replace your 12/13 and 13/13.
Thanks. It is good that I can drop that last one. Will replace
(with a SQUASH??? for 1/2).
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows
2017-05-26 3:35 ` [PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows Junio C Hamano
2017-05-29 20:25 ` [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames Johannes Sixt
@ 2017-05-30 19:13 ` Stefan Beller
2017-05-30 23:17 ` Junio C Hamano
1 sibling, 1 reply; 41+ messages in thread
From: Stefan Beller @ 2017-05-30 19:13 UTC (permalink / raw)
To: Junio C Hamano, Johannes Schindelin, Johannes Sixt; +Cc: git@vger.kernel.org
On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> When asked to open/fopen a path, e.g. "a/b:/c", which does not exist
> on the filesystem, Windows (correctly) fails to open it but sets
> EINVAL to errno
errno to EINVAL (as of now it sounds as if it is a EINVAL = errno,
which makes no sense to me)
> because the pathname has characters that cannot be
> stored in its filesystem.
>
> As this is an expected failure, teach is_missing_file_error() helper
> about this case.
>
> This is RFC,
cc'd people knowledgeable of Windows.
> as there may be a case where we get EINVAL from
> open/fopen for reasons other than "the filesystem does not like this
> pathname" that may be worth reporting to the user, and this change
> is sweeping such an error under the rug.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> wrapper.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/wrapper.c b/wrapper.c
> index f1c87ec7ea..74aa3b7803 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -434,6 +434,10 @@ static void warn_on_inaccessible(const char *path)
> * see if the errno indicates a missing file that we can safely ignore.
> */
> static int is_missing_file_error(int errno_) {
> +#ifdef GIT_WINDOWS_NATIVE
> + if (errno_ == EINVAL)
> + return 1;
> +#endif
> return (errno_ == ENOENT || errno_ == ENOTDIR);
> }
>
> --
> 2.13.0-491-g71cfeddc25
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows
2017-05-30 19:13 ` [PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows Stefan Beller
@ 2017-05-30 23:17 ` Junio C Hamano
2017-05-30 23:32 ` Stefan Beller
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2017-05-30 23:17 UTC (permalink / raw)
To: Stefan Beller; +Cc: Johannes Schindelin, Johannes Sixt, git@vger.kernel.org
Stefan Beller <sbeller@google.com> writes:
> On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> When asked to open/fopen a path, e.g. "a/b:/c", which does not exist
>> on the filesystem, Windows (correctly) fails to open it but sets
>> EINVAL to errno
>
> errno to EINVAL (as of now it sounds as if it is a EINVAL = errno,
> which makes no sense to me)
>
>> because the pathname has characters that cannot be
>> stored in its filesystem.
>>
>> As this is an expected failure, teach is_missing_file_error() helper
>> about this case.
>>
>> This is RFC,
>
> cc'd people knowledgeable of Windows.
This has been resolved I think with J6t/Dscho's patch yesterday.
Thanks.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows
2017-05-30 23:17 ` Junio C Hamano
@ 2017-05-30 23:32 ` Stefan Beller
0 siblings, 0 replies; 41+ messages in thread
From: Stefan Beller @ 2017-05-30 23:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Johannes Sixt, git@vger.kernel.org
On Tue, May 30, 2017 at 4:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>> cc'd people knowledgeable of Windows.
>
> This has been resolved I think with J6t/Dscho's patch yesterday.
>
> Thanks.
Yeah I saw those patches after reviewing this series.
Sorry for the noise,
Stefan
^ permalink raw reply [flat|nested] 41+ messages in thread