Johannes Schindelin [off-list ref] writes:
+core.hideDotFiles::
+ (Windows-only) If true, mark newly-created directories and files whose
+ name starts with a dot as hidden. If 'dotGitOnly', only the `.git/`
+ directory is hidden, but no other files starting with a dot. The
+ default mode is to mark only the `.git/` directory as hidden.
I think "The default mode is 'dotGitOnly'" is sufficient, given that
it is described just one sentence before, which is still likely to
be in readers' mind. But I'll let it pass without tweaking.
+enum hide_dotfiles_type {
+ HIDE_DOTFILES_FALSE = 0,
+ HIDE_DOTFILES_TRUE,
+ HIDE_DOTFILES_DOTGITONLY,
We allow ',' after the last array initializer, but not after the
last enum definition. I'll tweak it out while queuing.
quoted hunk
@@ -319,6 +364,21 @@ int mingw_open (const char *filename, int oflags, ...)
if (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY))
errno = EISDIR;
}
+ if ((oflags & O_CREAT) && needs_hiding(filename)) {
+ /*
+ * Internally, _wopen() uses the CreateFile() API which errors
+ * out with an ERROR_ACCESS_DENIED if CREATE_ALWAYS was
+ * specified and an already existing file's attributes do not
+ * match *exactly*. As there is no mode or flag we can set that
+ * would correspond to FILE_ATTRIBUTE_HIDDEN, let's just try
+ * again *without* the O_CREAT flag (that corresponds to the
+ * CREATE_ALWAYS flag of CreateFile()).
+ */
+ if (fd < 0 && errno == EACCES)
+ fd = _wopen(wfilename, oflags & ~O_CREAT, mode);
This "retry if we got EACCESS" felt strange to me in two ways. One
is explained well in the comment and you know what you are doing, as
opposed to me who is clueless with CreateFile() API.
The other is why you do not have to retry creation in a similar way
when !needs_hiding(filename). I didn't see anything in the function
before reaching this point that does anything differently based on
needs_hiding(). Can't the same 'ERROR_ACCESS_DENIED' error trigger
if CREATE_ALWAYS was specified and file attributes of an existing
file match, and if it can, don't you want to retry that too, even if
you are not going to hide the filename?
That is, I am wondering, without knowing much about Windows API, why
the code is more like this:
fd = _wopen(...);
if (fd < 0 && ...) {
if (attrs != INVALID_...)
errno = ISDIR;
}
if ((oflags & O_CREAT) && fd < 0 && errno == EACCESS)
/* That big comment here ... */
fd = _open(wfilename, oflags & ~O_CREAT, mode);
if ((oflags & O_CREAT) && needs_hiding(filename)) {
if (fd >= 0 && set_hidden_flag(wfilename, 1))
warning("could not mark '%s' as hidden"...);
}
Obviously, I will *not* do this tweak myself ;-)
+ if (fd >= 0 && set_hidden_flag(wfilename, 1))
+ warning("Could not mark '%s' as hidden.", filename);
+ }
I'll tweak all new instances of "Could" with s/Could/could/ to save
Micheal trouble (cf. b846ae21daf).
Thanks.
Hi Junio,
On Mon, 9 May 2016, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
+core.hideDotFiles::
+ (Windows-only) If true, mark newly-created directories and files whose
+ name starts with a dot as hidden. If 'dotGitOnly', only the `.git/`
+ directory is hidden, but no other files starting with a dot. The
+ default mode is to mark only the `.git/` directory as hidden.
I think "The default mode is 'dotGitOnly'" is sufficient, given that
it is described just one sentence before, which is still likely to
be in readers' mind. But I'll let it pass without tweaking.
Yeah, when rewriting this after Ramsay pointed out the erroneous
documentation, I wanted to fail on the crystal-clear side.
quoted
+enum hide_dotfiles_type {
+ HIDE_DOTFILES_FALSE = 0,
+ HIDE_DOTFILES_TRUE,
+ HIDE_DOTFILES_DOTGITONLY,
We allow ',' after the last array initializer, but not after the
last enum definition. I'll tweak it out while queuing.
Whoops. Sorry. I should have caught that five years ago.
quoted
@@ -319,6 +364,21 @@ int mingw_open (const char *filename, int oflags, ...)
if (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY))
errno = EISDIR;
}
+ if ((oflags & O_CREAT) && needs_hiding(filename)) {
+ /*
+ * Internally, _wopen() uses the CreateFile() API which errors
+ * out with an ERROR_ACCESS_DENIED if CREATE_ALWAYS was
+ * specified and an already existing file's attributes do not
+ * match *exactly*. As there is no mode or flag we can set that
+ * would correspond to FILE_ATTRIBUTE_HIDDEN, let's just try
+ * again *without* the O_CREAT flag (that corresponds to the
+ * CREATE_ALWAYS flag of CreateFile()).
+ */
+ if (fd < 0 && errno == EACCES)
+ fd = _wopen(wfilename, oflags & ~O_CREAT, mode);
This "retry if we got EACCESS" felt strange to me in two ways. One
is explained well in the comment and you know what you are doing, as
opposed to me who is clueless with CreateFile() API.
The other is why you do not have to retry creation in a similar way
when !needs_hiding(filename). I didn't see anything in the function
before reaching this point that does anything differently based on
needs_hiding(). Can't the same 'ERROR_ACCESS_DENIED' error trigger
if CREATE_ALWAYS was specified and file attributes of an existing
file match, and if it can, don't you want to retry that too, even if
you are not going to hide the filename?
The attributes that can trigger the ERROR_ACCESS_DENIED error are the
hidden flag and the system flag. Since Git *never* marks any file as
system file, we should be safe. Granted, it would be theoretically
possible that some enterprisey person tracks system files and marks them
via some hook or some such. I am not willing to introduce support for that
until somebody *actually* needs it and puts in the work to verify that it
will work as expected.
There is another case where ERROR_ACCESS_DENIED can be returned that is
worth mentioning: under certain circumstances, deleting a file does not
delete the file right away, but waits until the last handle to said file
was closed. In such a case, the user cannot create that file, either. It
does not exactly hurt to try without O_CREAT again, but it does not make
sense in this case, either.
And finally, there are programs that lock files when they access them (MS
Access, I am looking at you). I believe that we would end up with an
EACCES in that case, too, but we would not really be able to do much about
it.
We *could* introduce the same "Try again?" handling as we already have for
rename() and friends, but I'd rather get Karsten's clean-up patch
https://github.com/git-for-windows/git/commit/86e8394c2 in before doing
that. If we do it for fopen()/freopen()/open() at all.
quoted
+ if (fd >= 0 && set_hidden_flag(wfilename, 1))
+ warning("Could not mark '%s' as hidden.", filename);
+ }
I'll tweak all new instances of "Could" with s/Could/could/ to save
Micheal trouble (cf. b846ae21daf).
You mean Michael? ;-)
Anyway, the next iteration of this patch is on its way.
Ciao,
Dscho