Re: [PATCH v2 1/2] mingw: introduce the 'core.hideDotFiles' setting

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <johannes.schindelin@gmx.de> 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.

> @@ -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.