Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
>> In this case, the warning occurs because I build with nd/fopen-errors.
>
> Ah. So the base commit Junio chose for your v1 is completely
> inappropriate. It should be nd/fopen-errors instead.
Actuallly, Hannes's patch text and problem description are
confusingly inconsistent, and I have to say that the above statement
is wrong---do not react to this statement just yet, because I'll
also say "but you are right" real soon.
Because "git fetch a/b" your UNIXy friends run will not consider
"a/b" as a remote nickname, "a\b" in "git fetch a\b" Windows folks
run must not be taken as a remote nickname either. That is a
justification enough for Hannes's patch text, and that does not
change whether we take nd/fopen-errors or discard it. So in that
sense, the patch text does not have anything to do with the other
topic.
But you are right (as promised ;-) to say that nd/fopen-errors needs
a bit more polishing to work correctly on Windows. I unfortunately
do not think Hannes's patch addresses the real issue.
Isn't the root cause of "Invalid argument" a colon in a (mistakenly
identified) remote nickname, not a backslash? I doubt that it would
help to loosen valid_remote_nick() to pay attention to backslashes.
Surely, it may hide the issue when the path also has a backslash,
but wouldn't Windows folks see the same warning for "git fetch c:c"
for example even with Hannes's patch?
We use the pattern "try to open a path, and treat a failure to open
as indicating that the path is missing (and take information we
would have obtained from the contents of the path, if existed, from
elsewhere, as necessary)" pretty much everywhere, not just when
accessing a remote. And nd/fopen-errors tries to tweak the "treat a
failure to open as a sign of missing path" to be a bit more careful
by warning unless the error we get is ENOENT or ENOTDIR (otherwise
we may treat a file that exists but cannot be read as if it is
missing without any indication).
The solution for the problem Hannes's proposed log message describes
lies in warn_on_fopen_errors() function that was introduced in the
nd/fopen-errors topic. It appears that in Windows port, open() and
fopen() return EINVAL for a file that does not exist and whose name
Windows does not like, so we probably should do something like the
attached to work around the EINVAL (I do not know about the symbol
checked for #ifdef---there may be a more appropriate one).
I am not entirely happy with the workaround, because I do not know
if EINVAL can come _ONLY_ due to a colon in the pathname on Windows,
or if there are some other cases that deserve to be warned that also
yield EINVAL, and the attached will sweep the problem under the rug
for the "other cases", partially undoing what nd/fopen-errors topic
wanted to do. But it does not make it worse than before the topic
happened [*1*].
And that kind of solution does belong to nd/fopen-errors.
So in short:
(1) Hannes's patches are good, but they solve a problem that is
different from what their log messages say; the log message
needs to be updated;
(2) In nd/fopen-errors topic, we need a new patch that deals with
EINVAL on Windows.
Thanks.
[Footnote]
*1* An alternative that may allow us to avoid sweeping the issue
under the rug may be to have a more precise logic to see if the
open failure is due to missing file in the implementation of
open() and fopen() emulation---at that level, the logic can be
built with more platform knowledge (e.g. EINVAL? Let's see what
path we got---ah, there is a colon)---and turn EINVAL into
ENOENT, or something like that. I do not have a strong opinion
if that is a better solution or the attached workaround is good
enough.
wrapper.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/wrapper.c b/wrapper.c
index 6e513c904a..7b6d3e3f94 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -418,9 +418,18 @@ FILE *fopen_for_writing(const char *path)
return ret;
}
+static int is_an_error_for_missing_path(int no)
+{
+#if WINDOWS
+ if (errno == EINVAL)
+ return 1;
+#endif
+ return (errno == ENOENT || errno == ENOTDIR);
+}
+
int warn_on_fopen_errors(const char *path)
{
- if (errno != ENOENT && errno != ENOTDIR) {
+ if (!is_error_for_missing_path(errno)) {
warn_on_inaccessible(path);
return -1;
}