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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 6, 2016, 15:19 UTC
Message-ID: <alpine.DEB.2.20.1605061658580.2963@virtualbox>
In-Reply-To: <xmqqr3dhhcd7.fsf@gitster.mtv.corp.google.com>

Hi Junio,

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

Show 24 quoted lines

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

Show 9 quoted lines

> 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