Re: [PATCH] mingw: introduce the 'core.hideDotFiles' setting

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Wed, 4 May 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/builtin/init-db.c b/builtin/init-db.c
> > index b2d8d40..c4269ac 100644
> > --- a/builtin/init-db.c
> > +++ b/builtin/init-db.c
> > @@ -370,6 +370,7 @@ int init_db(const char *template_dir, unsigned int flags)
> >  	check_repository_format();
> >  
> >  	reinit = create_default_files(template_dir);
> > +	mark_as_git_dir(get_git_dir());
> >  
> >  	create_object_directory();
> >  
> 
> I agree with the goal of the change, but I am having a hard time
> justifying this addition.  Primarily because I do not understand the
> need for this.
> 
> In order to be prepared to handle HIDE_DOTFILES_TRUE case,
> mingw_mkdir() needs to be taught about needs_hiding() and
> make_hidden() already.  Why isn't it sufficient to teach
> needs_hiding() to check with !strcmp(basename(path, ".git"))
> under HIDE_DOTFILES_DOTGITONLY?

The reason was that I wanted to avoid to compare a name unnecessarily when
I already had a code path that knew perfectly well that a given directory
is the .git/ directory.

I made the change. It was more painful than I expected, as two bugs were
uncovered, both introduced after the original patch by Erik.

First bug: basename() on Windows used to not remove the trailing slashes.
Since we adjusted it, to conform with the POSIX specs, the implicit
mkdir() in copy_templates() could replace the trailing slash by a NUL,
breaking the template copying (it truncated pretty much all paths). I did
not catch this earlier because basename() was only called with
HIDE_DOTFILES_TRUE, and that case was never thoroughly tested.

So I had to reroll a non-modifying basename(). It's not elegant to have
this, but it is better than strdup()ing each and every path, just to
determine whether the basename starts with a dot (or is equal to ".git")
or not.

Second bug: when we switched to override open()/fopen()/freopen() using
Windows' UTF-16 functions, we lost the ability to open hidden files
(internally, _wopen() uses CreateFile(), which allows the equivalent of
O_CREAT only if the attributes match any existing files' attributes
*exactly*, and there is no way to tell _wopen() that we want to create a
hidden file).

Again, this was only exercised with HIDE_DOTFILES_TRUE until the change
proposed by you: needs_hiding() now also triggers for .git *files* in
HIDE_DOTFILES_DOTGITONLY mode.

It worries me slightly that the new code is so different from the code
that was tried and tested through all those years (although admittedly it
is unlikely anybody ever ran with core.hideDotFiles = true, given above
findings). But I guess that cannot be helped. Not unless we reintroduce
those two bugs.

> I do not understand why core.hideDotFiles needs to be explicitly
> copied to the config file in the resulting repository, either.
> 
> Once you created a repository, Git won't unhide the hidden .git of
> that reposiotry, so the idea must be to propagate the setting to a
> new repository that will further be created, but wouldn't that get
> the "please hide" preference from the same place as we have got the
> preference to hide .git when the current repository was created
> anyway?

I had a look in the mail archives, and it looks as if I wanted to support
`git clone -c core.hideDotFiles...`. I introduced a new regression test
and verified that we no longer need to write that config setting
explicitly.

I will send out v2 as soon as the test suite passed (which is hopefully
30-45 minutes from now).

Ciao,
Dscho