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

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

Hi Junio,

On Fri, 6 May 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> 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.
> > ...
> > 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 have a huge preference for a code that has been production for
> years over a new code that would cook at most two weeks in 'next'.

I agree. However, it does not fill me with confidence that we did not
catch those two bugs earlier. Even one round of reviews (including a
partial rewrite) was better than all that time since the regressions were
introduced.

Besides, your innocuous remark that needs_hiding() could determine whether
we are looking at ".git" revealed a problem with the original design:
there can be .git *files*. Support for .git files was introduced in
February 2008, so it is my fault that I did not catch this in January
2010, when I added the "dotGitOnly" option.

I do not think that we can fix this design other than abandoning the
mark_as_git_dir() function.

> As I said, the part regarding the mark_as_git_dir() in the message
> you are responding to was me asking you to explain, not me objecting
> to the code.

I understood. My initial reaction was: it makes a total lot of sense to
simplify the patch by removing that global.

It was more painful than anticipated only because I did not expect any
bugs to be revealed in the process, certainly not hard-to-debug ones (I
had to patch submodule--helper's code to allow attaching a debugger at a
very specific point during t1013 so I could single-step through it, and it
took quite a couple of iterations to pinpoint the problem).

Even as it was painful, it was useful, too, though, as bugs were revealed
and squashed. And an additional test was introduced and unnecessary code
was dropped.

In the same vein, was wondering whether we want to hide those Windows-only
core config options behind a platform_core_config() which would then be
#define'd to point to mingw_core_config()?

> So given the knowledge that
> 
>  - I am not fundamentally opposed to having an extra call there; - in
>  fact, I suspect it may even be a good thing to have one; - I am not
>  entirely happy with the name mark_as_git_dir; and - the rewrite to lose
>  that call was more painful than anticipated.
> 
> would you still choose to lose the extra call and go with
> !stricmp(basename(path), ".git")?  The best approach for v2 might be to
> 
>  - Keep the two bugfixes that was uncovered during this exercise; - keep
>  the change to init_db() to add a call to mark_as_git_dir(); -
>  optionally, come up with a better name for that function; and - drop
>  the setting of configuration varaibles that was unnecessary.

Well, given that I learned that I cannot use basename() but have to
partially copy its code, the simpler solution *is* to abandon the
mark_as_git_dir() approach.

See v2 (my apologies for sending it only today, I encountered a bug in my
patch series sending script and was unable to fix it yesterday).

Ciao,
Dscho