Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.

From: Madhu <enometh@meer.net>
Date: Mar 22, 2021, 09:04 UTC
Message-ID: <20210322.143437.212295420302618690.enometh@meer.net>
In-Reply-To: <xmqq7dlz94by.fsf@gitster.g>

*  Junio C Hamano <gitster@pobox.com> <xmqq7dlz94by.fsf@gitster.g>
Wrote on Sun, 21 Mar 2021 21:53:37 -0700

Show 24 quoted lines

> There are two points we should consider.
>
>  * Historically, we've used .git/config as the sample file to check,
>    but that was not because we wanted to make sure we can chmod the
>    config file, but because we knew the file has to be there.  If
>    .git/config is sometimes a symbolic link, and if chmod test
>    requires a regular file, we do not have to use .git/config as the
>    sample file.  We could instead switch to use a different,
>    temporary, file.  After all, the symlink check I pointed out in
>    the message you are responding to uses a brand new temporary
>    filename for that, and there is no reason why we shouldn't do the
>    same with a regular file for the filemode test.
>
>  * If running "git init" in an already functioning repository can be
>    a useful way to "re-initialize" and/or "correct" various aspect
>    of the repository (e.g. perhaps core.filemode is incorrectly set
>    originally and running "git init" again corrects it), we would
>    want to allow that in a normal repository as well as in a
>    repository that is created by new-workdir the same way.  Or if it
>    is not useful and we want "re-initialize" not to touch the
>    filemode, we would want to skip the check in a normal repository
>    as well as in a new-workdir repository the same way.  That is why
>    "if symlink, then skip" is wrong---it targets the new-workdir
>    case specifically.

I'd say it doesn't (target the new-workdir case specifically). The lstat test on `config' is incorrect if `config' (or a new file) is not a regular file, so I think it should be fixed anyway. The new-workdir case happens to be handled correctly once this is fixed.

Show 7 quoted lines

> I personally do not have a strong opinion either way, but to me, it
> seems that "does the filesystem support filemode?" and "does the
> filesystem support symbolic link?" are at about the same level and
> should be treated similarly unless there is a good reason not to.
> And the symlink check is never done in "reinit" case, so perhaps
> when "git init" is run again in an already functioning repository,
> we should not muck with the filemode, either.

I'd think so (on the last point). While it is understandable to expect consistent re-initing behaviour (which is why the spurious git-init was there) the expectations may not hold if the git directory and work tree are on different filesystems - there is more scope for making wrong inferences.

Typically checkout worktrees with new-workdir in /dev/shm for advantages in the low overhead.

I do have repositories where .git/config is a symlink to a config.A and I have other config.B files - all for the same repo but with different upstreams. (I know better ways to do it but why should I be prevented from doing this)

Show 13 quoted lines

> A natural conclusion of the line of thought is that we can move the
> "check filemode trustability" block (from the comment to concluding
> git_config_set()) inside the "if (!reinit)" that happens a bit later
> and we'd be fine---as an existing normal repository, as well as what
> new-workdir creates, won't have to do the "let's chmod +x/-x the
> config file and see what happens" code at all (perhaps the attached
> patch, which hasn't even been compile tested).
>
> On the other hand, if it is worth "fixing" the filemode setting
> while re-initializing, we probably should switch to use a temporary
> file instead of 'config'.  And we may want to reconsider the placement
> of the "is symlink supported?" check---which may also have to be
> redone to "fix" its existing value.

I don't think the posted patch (snipped) would work as reinit is always 1 and we are always a candidate for reiniting - I may be missing something.

Using a new file for the filemode test would be a natural improvement. But I dont see the added complexity as a win. I can imagine other scenarios that need to be fixed: calling git-init on a worktree prepared by git-new-worktree (not git-new-workdir) on a FAT fs with .git on ext2. Calling git-init on a worktree prepared by calling git-new-worktree with a parent which is checked out by git-new-workdir. (The parent has a symlinked config). The possibilities for fun endless :)