Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

From: Genki Sky <sky@genki.is>
To: Guenter Roeck <linux@roeck-us.net>,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	Christian Kujau <lists@nerdbynature.de>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"
Date: Tue, 06 Nov 2018 11:23:05 -0800	[thread overview]
Message-ID: <20181106.192305.406697677@genki.is> (raw)
In-Reply-To: <1541527838-4585-1-git-send-email-linux@roeck-us.net>

Hi Guenter,

On Tue,  6 Nov 2018 10:10:38 -0800, Guenter Roeck <linux@roeck-us.net> wrote:
> This reverts commit 6147b1cf19651c7de297e69108b141fb30aa2349.
>
> The reverted patch results in attempted write access to the source
> repository, even if that repository is mounted read-only.
>
> Output from "strace git status -uno --porcelain":
>
> getcwd("/tmp/linux-test", 129)          = 16
> open("/tmp/linux-test/.git/index.lock", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) =
> 	-1 EROFS (Read-only file system)
>
> While git appears to be able to handle this situation, a monitored build
> environment (such as the one used for Chrome OS kernel builds) may detect
> it and bail out with an access violation error. On top of that, the attempted
> write access suggests that git _will_ write to the file even if a build output
> directory is specified. Users may have the reasonable expectation that the
> source repository remains untouched in that situation.

Hmm, so in summary: According to 6147b1cf1965
("scripts/setlocalversion: git: Make -dirty check more robust",
2018-08-28), one scenario requires the index to be refreshed to get a
correct "dirty" or "not dirty" status. But according to your commit
here, another scenario requires the kernel build system to not even
attempt to update the git index, and doesn't care / aren't impacted by
the cases where the index needs to be refreshed.

Perhaps both scenarios could be satisfied by having
scripts/setlocalversion first check if .git has write permissions, and
acting accordingly. Looking into history, this actually used to be
done, but cdf2bc632ebc ("scripts/setlocalversion on write-protected
source tree", 2013-06-14) removed the updating of the index.

However, I admit I don't understand the justification in that commit
from 2013. I'm no NFS expert, but perhaps the real problem there is an
incorrectly configured NFS setup (uid/gid mismatch between NFS
client/server, or permissions mismatch between mount options and NFS
server?). Christian Kujau: can you speak to that?

Well, we could also make our check $(touch .git/some-file-here
2>/dev/null && ...) instead of $(test -w .git) to handle misconfigured
NFS setups. But not sure if that has its own problems.

Thoughts? It'd be nice to find a fix that works for everyone.

Genki