Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths

[Johannes Sixt]

Am 24.05.2017 um 00:08 schrieb Junio C Hamano:

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.


Will reroll the patches. Good analysis, BTW. (It was a late discovery of 
mine that nd/fopen-errors is actually the source of the warnings.)


-- Hannes

Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths

[Junio C Hamano]

Johannes Schindelin  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;
}

Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths

[Johannes Sixt]

Am 23.05.2017 um 12:53 schrieb Johannes Schindelin:

Hi Hannes (& Junio, see below),

On Mon, 22 May 2017, Johannes Sixt wrote:


Am 22.05.2017 um 13:59 schrieb Johannes Schindelin:

On Sat, 20 May 2017, Johannes Sixt wrote:

This small series fixes these warnings on Windows:

C:\Temp\gittest>git fetch C:\Temp\gittest
warning: unable to access '.git/remotes/C:\Temp\gittest': Invalid argument
warning: unable to access '.git/branches/C:\Temp\gittest': Invalid
warning: argument
  From C:\Temp\gittest
   * branchHEAD   -> FETCH_HEAD

The fix is in the second patch; the first patch is a
preparation that allows to write the fix in my preferred style.


Thank you!

Maybe you want to accompany these patches with a simple test case that
uses e.g. ls-remote on $(pwd | tr / )?


Actually, no, I don't want to. It would have to be protected by MINGW, and I
don't want to burden us (and here I mean Windows folks) with a check for a
cosmetic deficiency. (Shell scripts, slow forks, yadda, yadda...)


Actually, yes, I want to.

Yes, it would have to be protected by MINGW, and yes, it would put a
burden on us, but also yes: it would put an even higher burden on me to
check this manually before releasing Git for Windows, or even worse: this
regression would be the kind of bug that triggers many bug reports,
addressing which would take a lot more time than writing this test case
and executing it as part of our test suite.


Fair enough. The patch looks good. I'll be able to test no earlier than 
Monday, though.




So here goes:

-- snipsnap --
From: Johannes Schindelin 
Date: Tue, 23 May 2017 12:42:13 +0200
Subject: [PATCH] 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).

Signed-off-by: Johannes Schindelin 
---
  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 b195f71ea98..93ce99ba3c6 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_success '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]:*)

Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths

[Johannes Schindelin]

Hi Hannes (& Junio, see below),

On Mon, 22 May 2017, Johannes Sixt wrote:

> Am 22.05.2017 um 13:59 schrieb Johannes Schindelin:
> > On Sat, 20 May 2017, Johannes Sixt wrote:
> > > This small series fixes these warnings on Windows:
> > >
> > > C:\Temp\gittest>git fetch C:\Temp\gittest
> > > warning: unable to access '.git/remotes/C:\Temp\gittest': Invalid argument
> > > warning: unable to access '.git/branches/C:\Temp\gittest': Invalid
> > > warning: argument
> > >  From C:\Temp\gittest
> > >   * branchHEAD   -> FETCH_HEAD
> > >
> > > The fix is in the second patch; the first patch is a
> > > preparation that allows to write the fix in my preferred style.
> > 
> > Thank you!
> > 
> > Maybe you want to accompany these patches with a simple test case that
> > uses e.g. ls-remote on $(pwd | tr / )?
> 
> Actually, no, I don't want to. It would have to be protected by MINGW, and I
> don't want to burden us (and here I mean Windows folks) with a check for a
> cosmetic deficiency. (Shell scripts, slow forks, yadda, yadda...)

Actually, yes, I want to.

Yes, it would have to be protected by MINGW, and yes, it would put a
burden on us, but also yes: it would put an even higher burden on me to
check this manually before releasing Git for Windows, or even worse: this
regression would be the kind of bug that triggers many bug reports,
addressing which would take a lot more time than writing this test case
and executing it as part of our test suite.

So here goes:

-- snipsnap --
From: Johannes Schindelin 
Date: Tue, 23 May 2017 12:42:13 +0200
Subject: [PATCH] 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).

Signed-off-by: Johannes Schindelin 
---
 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 b195f71ea98..93ce99ba3c6 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_success '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.windows.1

Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths

[Johannes Schindelin]

Hi Hannes,

On Mon, 22 May 2017, Johannes Sixt wrote:

> Am 22.05.2017 um 16:01 schrieb Johannes Schindelin:
> > On Mon, 22 May 2017, stefan.na...@atlas-elektronik.com wrote:
> > > Am 20.05.2017 um 08:28 schrieb Johannes Sixt:
> > > > This small series fixes these warnings on Windows:
> > > >
> > > > C:\Temp\gittest>git fetch C:\Temp\gittest
> > > > warning: unable to access '.git/remotes/C:\Temp\gittest': Invalid
> > > > warning: argument
> > > > warning: unable to access '.git/branches/C:\Temp\gittest': Invalid
> > > > warning: argument
> > > >  From C:\Temp\gittest
> > > >   * branchHEAD   -> FETCH_HEAD
> > >
> > > What is the exact precondition to get such a warning ?
> > >
> > > Just wondering, because I'm not able to reproduce that warning
> > > (with Git4win version 2.13.0.windows.1).
> > 
> > I had tested this also, and came to the conclusion that only Hannes'
> > MSys-based custom version is affected that is much closer to git.git's
> > `master` than to Git for Windows' fork.
> 
> 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.

Thanks for the clarification!
Dscho

Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths

[Johannes Sixt]

Am 22.05.2017 um 13:59 schrieb Johannes Schindelin:

On Sat, 20 May 2017, Johannes Sixt wrote:

This small series fixes these warnings on Windows:

C:\Temp\gittest>git fetch C:\Temp\gittest
warning: unable to access '.git/remotes/C:\Temp\gittest': Invalid argument
warning: unable to access '.git/branches/C:\Temp\gittest': Invalid argument
 From C:\Temp\gittest
  * branchHEAD   -> FETCH_HEAD

The fix is in the second patch; the first patch is a
preparation that allows to write the fix in my preferred style.


Thank you!

Maybe you want to accompany these patches with a simple test case that
uses e.g. ls-remote on $(pwd | tr / )?


Actually, no, I don't want to. It would have to be protected by MINGW, 
and I don't want to burden us (and here I mean Windows folks) with a 
check for a cosmetic deficiency. (Shell scripts, slow forks, yadda, 
yadda...)


-- Hannes

Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths

[Johannes Sixt]

Am 22.05.2017 um 16:01 schrieb Johannes Schindelin:

On Mon, 22 May 2017, stefan.na...@atlas-elektronik.com wrote:

Am 20.05.2017 um 08:28 schrieb Johannes Sixt:

This small series fixes these warnings on Windows:

C:\Temp\gittest>git fetch C:\Temp\gittest
warning: unable to access '.git/remotes/C:\Temp\gittest': Invalid argument
warning: unable to access '.git/branches/C:\Temp\gittest': Invalid argument
 From C:\Temp\gittest
  * branchHEAD   -> FETCH_HEAD


What is the exact precondition to get such a warning ?

Just wondering, because I'm not able to reproduce that warning
(with Git4win version 2.13.0.windows.1).


I had tested this also, and came to the conclusion that only Hannes'
MSys-based custom version is affected that is much closer to git.git's
`master` than to Git for Windows' fork.


In this case, the warning occurs because I build with nd/fopen-errors.

Which explains why I observed it only recently even though I fetch from 
Windows paths regularly.


-- Hannes

Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths

[Johannes Schindelin]

Hi Stefan,

On Mon, 22 May 2017, stefan.na...@atlas-elektronik.com wrote:

> Am 20.05.2017 um 08:28 schrieb Johannes Sixt:
> > This small series fixes these warnings on Windows:
> > 
> > C:\Temp\gittest>git fetch C:\Temp\gittest
> > warning: unable to access '.git/remotes/C:\Temp\gittest': Invalid argument
> > warning: unable to access '.git/branches/C:\Temp\gittest': Invalid argument
> > From C:\Temp\gittest
> >  * branchHEAD   -> FETCH_HEAD
> 
> What is the exact precondition to get such a warning ?
> 
> Just wondering, because I'm not able to reproduce that warning
> (with Git4win version 2.13.0.windows.1).

I had tested this also, and came to the conclusion that only Hannes'
MSys-based custom version is affected that is much closer to git.git's
`master` than to Git for Windows' fork.

Still, the patches are good in my opinion.

Ciao,
Dscho

Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths

[stefan.naewe]

Am 20.05.2017 um 08:28 schrieb Johannes Sixt:
> This small series fixes these warnings on Windows:
> 
> C:\Temp\gittest>git fetch C:\Temp\gittest
> warning: unable to access '.git/remotes/C:\Temp\gittest': Invalid argument
> warning: unable to access '.git/branches/C:\Temp\gittest': Invalid argument
> From C:\Temp\gittest
>  * branchHEAD   -> FETCH_HEAD

What is the exact precondition to get such a warning ?

Just wondering, because I'm not able to reproduce that warning
(with Git4win version 2.13.0.windows.1).

Stefan
-- 

/dev/random says: Spock, you are such a putz!
python -c "print 
'73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
 
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9  9666 829B 49C5 9221 27AF

Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths

[Johannes Schindelin]

Hi Hannes,


On Sat, 20 May 2017, Johannes Sixt wrote:

> This small series fixes these warnings on Windows:
> 
> C:\Temp\gittest>git fetch C:\Temp\gittest
> warning: unable to access '.git/remotes/C:\Temp\gittest': Invalid argument
> warning: unable to access '.git/branches/C:\Temp\gittest': Invalid argument
> From C:\Temp\gittest
>  * branchHEAD   -> FETCH_HEAD
> 
> The fix is in the second patch; the first patch is a
> preparation that allows to write the fix in my preferred style.

Thank you!

Maybe you want to accompany these patches with a simple test case that
uses e.g. ls-remote on $(pwd | tr / )?

Ciao,
Dscho

[PATCH 0/2] Fix warnings on access of a remote with Windows paths

[Johannes Sixt]

This small series fixes these warnings on Windows:

C:\Temp\gittest>git fetch C:\Temp\gittest
warning: unable to access '.git/remotes/C:\Temp\gittest': Invalid argument
warning: unable to access '.git/branches/C:\Temp\gittest': Invalid argument
>From C:\Temp\gittest
 * branchHEAD   -> FETCH_HEAD

The fix is in the second patch; the first patch is a
preparation that allows to write the fix in my preferred style.

Johannes Sixt (2):
  mingw.h: permit arguments with side effects for is_dir_sep
  Windows: do not treat a path with backslashes as a remote's nick name

 compat/mingw.h | 6 +-
 remote.c   | 5 -
 2 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.13.0.55.g17b7d13330