Hi Junio,
On Mon, 9 May 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@xxxxxx> 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.
Yeah, when rewriting this after Ramsay pointed out the erroneous
documentation, I wanted to fail on the crystal-clear side.
> > +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.
> > @@ -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.
> > + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html