Fix append failure issue under remote directories #2753

[sunzhuoshi]

Signed-off-by: sunzhuoshi sunzhuoshi@gmail.com

[dscho]

This is a great start!

I would like to ask for a few improvements:

• could you have a look at the existing commit messages, e.g. 9d0a094? Then try to imitate the style:
  • start with a very brief description that explains what this patch is all about (take c2369bd for inspiration)
  • then start the body of the commit message by describing the actual problem that is solved by this patch
  • continue by discussing the various alternatives you considered to solve this problem
  • explain why the approach taken by this patch was chosen
  • then, and only then, reference the ticket that is fixed by this patch
• this definitely needs a test case, if we can add one without requiring a "real" remote share. As I had described, we already use a trick in t/t5580-unc-paths.sh to test Git access to remote shares by using the so-called administrative share \\localhost\C$ which simply accesses the C: drive and is available in our CI builds.
  • if you are unsure how to write that test case, first have a look at https://github.com/git-for-windows/git/wiki/Running-Git's-regression-tests#running-individual-tests to run t/t5580-unc-paths.sh in your local checkout
  • once you ran this successfully, study the code, see for example how we test cloning from a remote share by using the $UNCPATH variable which simply contains the UNC version of the current directory's path
  • you will most likely want to add your test case at the very end of the test script, starting with test_expect_success and then continuing with a description of what is being tested, then the actual code to run for the test
  • to run your test, use the --run=9 option (with whatever number your test case will have, use --run=0 to find out, this will skip all tests but print out their labels and their numbers)
  • you will want to make extra sure that the test case fails unless you modify compat/mingw.c: after committing all your (in-flux) changes, temporarily revert the respective part by running git checkout HEAD^ -- compat/mingw.c, then building with make, then running the test (and when the test failed as expected, do not forget to undo the temporary revert via git checkout HEAD -- compat/mingw.c!)

[dscho]

This is a pretty costly call. It would be better to use something like convert_slashes().

[sunzhuoshi]

This is a pretty costly call. It would be better to use something like convert_slashes().

yeah, that is true, but GetDriveTypeW needs full path as its input.
Currently, I use GetDriveType to check if a file path is local or not and I failed to find a solid better solution, do you have any suggestion?
by the way, has_dos_drive_prefix should not work, for file may locate in a mapped network drive, such as drive "Z" mapped to a network folder.

[dscho]

Previously, this condition triggered when appending to anything but a local named pipe. Now, this is restricted to local disk files? That sounds a bit overzealous.

It would probably make more sense to trigger the new (expensive!) code path only if the open_fn() call further down in this function failed (essentially: try running with mingw_open_append() first, and re-try using _wopen() only if it failed, and only if GetLastError() has the expected value, which you can easily find out by printing it when the function call fails in your scenario, and then only if that GetDriveTypeW() indicates a remote path).

Of course, an alternative would be to simply ask has_dos_drive_prefix() as an indicator for a local file:

if ((oflags & O_APPEND) && !is_local_named_pipe_path(filename) && has_dos_drive_prefix(filename))

That should be good-enough in your use case, and fix the issue reliably without having to go through expensive Win32 API calls.

[sunzhuoshi]

After re-checking git commit(d641097) and MSDN doc(https://docs.microsoft.com/en-us/windows/win32/fileio/file-access-rights-constants), mingw_open_append is introduced for atomatic append, it uses FILE_APPEND_DATA only flag to achieve this, and according to the MSDN doc, it should only work for local files:

For a file object, the right to append data to the file. (For local files, write operations will not overwrite existing data if this flag is specified without FILE_WRITE_DATA.) For a directory object, the right to create a subdirectory (FILE_ADD_SUBDIRECTORY).

I totally agree with you on call mingw_open_append() first, then re-try _wopen() only if it failed. but I failed to do this based on my test, mingw_open_append will still succeed on a UNC path with FILE_APPEND_DATA, but it will fail in later write calls. here is my test code, please help to check:
https://gist.github.com/sunzhuoshi/3b3a82e23f0b0cdaf750c7755b843aee
by the way, I map driver "Z" in code to a network folder.

[dscho]

mingw_open_append will still succeed on a UNC path with FILE_APPEND_DATA, but it will fail in later write calls.

What is the error?

Can you reproduce the same error when using the "administrative share" \\localhost\c$?

[sunzhuoshi]

thank you for your feedback.
GetLastError() after write returns: ERROR_INVALID_PARAMETER : The parameter is incorrect.
and I failed to reproduce it when using \\localhost\c$ , it creates and writes file successfully.

[dscho]

GetLastError() after write returns: ERROR_INVALID_PARAMETER : The parameter is incorrect.

Okay. Unfortunately, this is not really helpful.

The problem I see here is the adverse effects of this PR for the common case: while is_local_named_pipe_path() only looks at the string and is therefore very fast, the new is_local_disk_file() function calls not only GetFullPathNameW() but also GetDriveTypeW(), i.e. two I/O operations, read: comparatively expensive functions.

Granted, this is "only" when we're opening a file with the intention to append to it. But still, it is a complication, and judging by the number of bug reports we received, the fix is actually not necessary for all network shares, therefore the workaround is a bit overzealous.

How about doing something different and going for a config option instead? Something like this:

static int append_atomically = -1;

[...]

if (append_atomically < 0 &&
    git_config_get_bool("windows.appendatomically", &append_atomically))
        append_atomically = 1; /* default to `true` */

if (append_atomically &&
    (oflags & O_APPEND) && !is_local_named_pipe_path(filename))
        open_fn = mingw_open_append;
[...]

We could then detect the ERROR_INVALID_PARAMETER condition and suggest to disable atomic appending via the config option.

That would get us mostly to the best of both worlds: best performance for the common case, and a way out of the problematic scenario via an opt-in config setting.

What do you think?

[sunzhuoshi]

I agree with you. I'll update it in one or two weeks. Sorry for busy these days.

[dscho]

Thank you!

[nana2three]

(invalid)

[dscho]

Very nice!

I would like to ask for just a few favors:

1. could you squash the two commits into a single one? (In the Git project, we frown upon introducing a change only to revert it within the same PR/patch series. I would recommend using an interactive rebase, changing the second commit's pick to a squash.)
2. could you wrap the commit message to <= 76 columns per line?
3. the git_config_get_bool() function returns 0 if it succeeded, we should set append_atomically only when it failed. Something like if (append_atomically < 0 && (!the_repository || git_config_get_bool(...))) append_atomically = 1;, I think.
4. we need to document this. To this end, we need to introduce Documentation/config/windows.txt and include it in Documentation/config.txt. See Documentation/config/safe.txt for an example to imitate (in particular the syntax, so that the manual page renders correctly).

[dscho]

Did you still want to add handling for the ERROR_INVALID_PARAMETER condition (suggesting to set windows.appendAtomically) as I suggested here?

[dscho]

Please use appendAtomically in the documentation, it is easier to read.

[sunzhuoshi]

sorry, I missed it, let me try.

[dscho]

Sorry for the delay!

In addition to the suggested change, could I ask you to sign off with your real name, i.e. those beautiful characters instead of a Latinized form?

[dscho]

"atomatic" -> "automatic"

[sunzhuoshi]

It should be "atomic", right?

[sunzhuoshi]

what do you mean by my real name? my Chinese name? or just "Sun Zhuoshi"?

[dscho]

I meant the Chinese name ;-) I just really like seeing those beautiful Chinese characters.

[dscho]

It should be "atomic", right?

Whoops, yes, you're correct, my bad!

[sunzhuoshi]

OK, but you still pointed out my mistake: "atomatic" -> "atomic". Sharp eyes!
No problem, next time, I'll try to sign off with my Chinese name.

[dscho]

Could we:

• use warning() here?
• guard this behind a check that we're looking at a remote filesystem (does GetFileType(h) return FILE_TYPE_REMOTE in that case)?

[sunzhuoshi]

Good idea! I'll try to update it.

[dscho]

@sunzhuoshi I hope you don't mind, I just force-pushed another change on top and plan on merging it as soon as the PR build passed. Could you have a quick look?

[sunzhuoshi]

OK, no problem.

[sunzhuoshi]

It passed my local test, pretty code update!

[dscho]

Awesome. Thank you for contributing, @sunzhuoshi !

[sunzhuoshi]

I learned a lot from you, thank you!