gittup
dllinject: Track actual attribute writes instead of the requesting flag
Opening a file with the FILE_WRITE_ATTRIBUTES access right merely requests write access to attributes, and does not constitute a write in itself.
In particular, this caused a problem with MS-DOS Player, which needs to open every file with FILE_WRITE_ATTRIBUTES. DOS does not require such a flag for changing a file's mtime through a previously opened file handle, so an emulator needs to open every file with this flag just in case the emulated DOS process decides to change file times later.
Using MS-DOS Player to run Turbo C++ 4.0 through Tup then caused this to happen:
*** tup messages ***
tup error: File 'D:/DEV/BUILD/BORLAND/TC4/BIN/dpmi16bi.ovl' was written to, but is not in .tup/db. You probably should specify it as an output
-- Delete: D:/DEV/BUILD/BORLAND/TC4/BIN/dpmi16bi.ovl
tup error: File 'D:/DEV/BUILD/BORLAND/TC4/BIN/rtm.exe' was written to, but is not in .tup/db. You probably should specify it as an output
-- Delete: D:/DEV/BUILD/BORLAND/TC4/BIN/rtm.exe
tup error: File 'D:/DEV/BUILD/BORLAND/TC4/BIN/TCC.EXE' was written to, but is not in .tup/db. You probably should specify it as an output
-- Delete: D:/DEV/BUILD/BORLAND/TC4/BIN/TCC.EXE
tup error: File 'D:/DEV/BUILD/BORLAND/TC4/INCLUDE/stdio.h' was written to, but is not in .tup/db. You probably should specify it as an output
-- Delete: D:/DEV/BUILD/BORLAND/TC4/INCLUDE/stdio.h
tup error: File 'D:/DEV/BUILD/BORLAND/TC4/INCLUDE/_defs.h' was written to, but is not in .tup/db. You probably should specify it as an output
-- Delete: D:/DEV/BUILD/BORLAND/TC4/INCLUDE/_defs.h
tup error: File 'D:/DEV/BUILD/BORLAND/TC4/INCLUDE/_nfile.h' was written to, but is not in .tup/db. You probably should specify it as an output
-- Delete: D:/DEV/BUILD/BORLAND/TC4/INCLUDE/_nfile.h
tup error: File 'D:/DEV/BUILD/BORLAND/TC4/INCLUDE/_null.h' was written to, but is not in .tup/db. You probably should specify it as an output
-- Delete: D:/DEV/BUILD/BORLAND/TC4/INCLUDE/_null.h
None of these were actually written to or had their date changed.
Tup then does in fact delete all files from outside the source tree that MS-DOS Player loaded with FILE_WRITE_ATTRIBUTES. In this case, this removes the build tool's executable and any #included standard library headers, forcing you to either reinstall the build tool or recover the missing files from a backup. (Thankfully, write-protecting the compiler's directory prevents this from succeeding!)
This change should still address the node.js use case that initially prompted the check for FILE_WRITE_ATTRIBUTES in c7160c8. I couldn't find a definitive list of everything that counts as "attributes", but it does seem to be limited to the timestamps and attribute bits covered by the FILE_BASIC_INFORMATION structure.
Thanks for the fix! Seems like a reasonable workaround, though it does make some current test cases fail, so I haven't merged it yet. t5068 fails, but I think it's just because the 'touch' command now generates two file file write notifications, so the warning gets displayed twice. That can probably be fixed in the test case itself if it's intended.
The other problem is t5091 fails, for a reason I don't fully understand yet. The error message I see is:
--- Run t5091-ignore-output.sh ---
.tup repository initialized: .tup/db
[ tup ] [0.001s] Scanning filesystem...
[ tup ] [0.003s] Reading in new environment variables...
[ tup ] [0.004s] Parsing Tupfiles...
0) [0.001s] .
[ ] 100%
[ tup ] [0.006s] No files to delete.
[ tup ] [0.007s] Generating .gitignore files...
[ tup ] [0.007s] Executing Commands...
0) [0.116s] sh run.sh
[ ] 100%
[ tup ] [0.127s] Updated.
*** create_list:
1
*** Nodes shouldn't have flags set
*** t5091-ignore-output.sh failed
One weird thing I see in the debug log (if the CFLAGS += -NDEBUG line is commented out in src/dllinject/Tupfile) is a bunch of extra NtCreateFile calls with the special file '??\MountPointManager'. I'm not sure if that's related to the top-level directory (tupid == 1) getting put back into the create_list or not. Any idea why that would show up with your change?
the 'touch' command now generates two file file write notifications, so the warning gets displayed twice. That can probably be fixed in the test case itself if it's intended.
I think it would be better to just not print the duplicate notification? update_write_info() is already supposed to filter duplicates, but this code no longer runs for hidden file writes as of 7149db1 due to the goto that jumps to the end of the loop. Not sure about the best fix here.
The other problem is t5091 fails, for a reason I don't fully understand yet.
Turns out that this was caused by the touch ignoredir command sending a file write event for the ignoredir directory, which caused the DB node to be turned from a directory to a file. Everything else in dllinject ignores directories too, so it makes sense to do the same for attribute changes. Adjusted the code accordingly.
One weird thing I see in the debug log (if the CFLAGS += -NDEBUG line is commented out in src/dllinject/Tupfile) is a bunch of extra NtCreateFile calls with the special file '??\MountPointManager'. I'm not sure if that's related to the top-level directory (tupid == 1) getting put back into the create_list or not. Any idea why that would show up with your change?
These come from the extra calls to GetFinalPathNameByHandleW(), which needs the MountPointManager to resolve drive letters.