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

From: Junio C Hamano <gitster@pobox.com>
Date: May 6, 2016, 16:34 UTC
Message-ID: <xmqqh9eb5eo0.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1605061658580.2963@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 22 quoted lines

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

If you are sure we do not need that, that is one less reason we would be better off without mark_as_git_dir(). It was another way how a $GIT_DIR creation codepath behaved differently from other mingw_mkdir() codepath in the patch.

Having said that, I actually was leaning towards an opinion that it might actually be better to have mark_as_git_dir() there. Platforms may need to do other things in their implementation of the function to tweak things inside $GIT_DIR, just like you had to have a place to add configuration variables and mark_as_git_dir() was the perfect place for it.

But mark_as_git_dir() is not a generic enough name to express its purpose. It wasn't even when all it did was to see the HIDE_DOTFILES configuration and hide it (you are not marking it, in the sense that after you return, you cannot tell which directories are "marked" as "git_dir" by only looking at the resulting filesystem entities), and as an all-purpose place to hook platform specific tweaks, it is even less about "marking it as a $GIT_DIR".

A name that hints the fact that it is a place to do a platform specific extra preparation of $GIT_DIR would be more appropriate.

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.

That is what I think, with the new knowledge I learned from your message.